Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix turning off autofix for sources containing scoped disable comments #4705

Merged
merged 3 commits into from Apr 21, 2020

Conversation

jeddy3
Copy link
Member

@jeddy3 jeddy3 commented Apr 15, 2020

Which issue, if any, is this issue related to?

Closes #4704

Is there anything in the PR that needs further explanation?

We were only disabling autofix on files containing unscoped disable comments. This pull request disables it for both scoped and unscoped.

@amyspark
Copy link

amyspark commented Apr 16, 2020

This version of stylelint works correctly with my toolchain (npm i https://github.com/stylelint/stylelint/tarball/issue-4704).

*
* @param {PostcssResult} postcssResult
* @returns {boolean}
*/
function isFixCompatible({ stylelint }) {
// Check for issue #2643
if (stylelint.disabledRanges.all.length) return false;
const hasUnscopedDisabledRanges = stylelint.disabledRanges.all.length > 0;
const hasScopedDisabledRanges = Object.keys(stylelint.disabledRanges).length > 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can turn off autofix for each scoped rule.
Change the scope data to be passed to the third argument of ruleFunction.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can turn off autofix for each scoped rule.

Am I right in thinking that would give us a more granular workaround where autofix is turned off (for the entire source):

  • for all rules, if the source contains unscoped disable comments
  • for only the applicable rules, if the source only contains scoped disable comments

Would you mind pulling down the pull request and showing me what you mean? And is it something we should add to this pull request or a follow-up?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@ota-meshi ota-meshi Apr 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The color-hex-length rule does not autofix. However, the indent rule does autofix.

https://github.com/stylelint/stylelint/compare/issue-4704..issue-4704-2#diff-01cbf9c2079c6adc0ce42f035fd16a45R245

Copy link
Member Author

@jeddy3 jeddy3 Apr 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a much better approach, thanks!

If possible, can you:

  • use require('common-tags').stripIndent to indent the indentation rule code snippets, like so:
                          .lint({
				code: stripIndent`
                                     /* stylelint-disable color-hex-length */
                                    a {
                                       color: #ffffff;
                                    }
                                `,
  • add a comment above the following:
// Next two conditionals are temporary measures until #2643 is resolved
isFileFixCompatible &&
!postcssResult.stylelint.disabledRanges[ruleName],

As their intent isn't immediately clear.

We should then merge your changes into this branch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed it and merged it into this branch.

Copy link
Member Author

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent. LGTM, thanks.

@jeddy3 jeddy3 mentioned this pull request Apr 19, 2020
6 tasks
Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice solution!

@jeddy3 jeddy3 requested a review from hudochenkov April 20, 2020 17:36
@jeddy3
Copy link
Member Author

jeddy3 commented Apr 20, 2020

I've added a commit to this pull request to update the docs to reflect the new behaviour introduced by this fix. I believe that autofix is now useful and stable for the majority of users. Those using scoped disable comments are only slightly inconvenienced when using the fix option. The minority of users using unscoped disable comments are more inconvenienced, but fix remains safe for them to use.

Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't realize how cool this PR until the last comment and document update. Amazing!

@jeddy3 jeddy3 merged commit 1e38d04 into master Apr 21, 2020
@jeddy3 jeddy3 deleted the issue-4704 branch April 21, 2020 11:25
@jeddy3
Copy link
Member Author

jeddy3 commented Apr 21, 2020

  • Fixed: autofix will respect scoped disable comments by turning off autofix for the scoped rules for the entire source; this is a continuation of the workaround added in 13.2.0 (#4705).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Fix autofix running on files containing scoped disable comments
4 participants