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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Apr 21, 2020
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d93bf67
Fix turning off autofix for sources containing scoped disable comments
jeddy3 Apr 15, 2020
9039766
Change to turn off autofix for each rule.
ota-meshi Apr 17, 2020
7af42d2
Update fix docs
jeddy3 Apr 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 4 additions & 3 deletions lib/lintSource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,15 +251,16 @@ function createEmptyPostcssResult(filePath) {
* There are currently some bugs in the autofixer of Stylelint.
* The autofixer does not yet adhere to stylelint-disable comments, so if there are disabled
* ranges we can not autofix this document. More info in issue #2643.
* Also, if this document is parsed with postcss-jsx and there are nested template
* literals, it will duplicate some code. More info in issue #4119.
*
* @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.

Hi @jeddy3.
I want to fix it like this:

https://github.com/stylelint/stylelint/compare/issue-4704..issue-4704-2

Copy link
Member

@ota-meshi ota-meshi Apr 17, 2020 •
edited

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 •
edited

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.


if (hasUnscopedDisabledRanges || hasScopedDisabledRanges) return false;

return true;
}
8 changes: 4 additions & 4 deletions lib/rules/block-closing-brace-newline-after/__tests__/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,10 @@ testRule(rule, {
code: '@media print { a { color: pink; }}\n@media screen { b { color: red; }}',
},
{
code: '.a {} /* stylelint-disable-line block-no-empty */',
code: '.a {} /* comment */',
},
{
code: '.a {} /* stylelint-disable-line block-no-empty */\n b {}',
code: '.a {} /* comment */\n b {}',
},
{
code: ':root {\n --x { color: pink; };\n --y { color: red; };\n }',
Expand Down Expand Up @@ -109,8 +109,8 @@ testRule(rule, {
column: 35,
},
{
code: '.a {} /* stylelint-disable-line block-no-empty */ b {}',
fixed: '.a {} /* stylelint-disable-line block-no-empty */\n b {}',
code: '.a {} /* comment */ b {}',
fixed: '.a {} /* comment */\n b {}',
message: messages.expectedAfter(),
line: 1,
column: 6,
Expand Down
47 changes: 47 additions & 0 deletions system-tests/fix/fix.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,53 @@ describe('fix', () => {
expect(result.output).toBe(code);
});
});

it("doesn't fix with scoped stylelint-disable commands", () => {
const code = `
/* stylelint-disable indentation */
a {
color: red;
}
`;

return stylelint
.lint({
code,
config: {
rules: {
indentation: 2,
},
},
fix: true,
})
.then((result) => {
expect(result.output).toBe(code);
});
});

it("doesn't fix with multiple scoped stylelint-disable commands", () => {
const code = `
/* stylelint-disable indentation, color-hex-length */
a {
color: #ffffff;
}
`;

return stylelint
.lint({
code,
config: {
rules: {
indentation: 2,
'color-hex-length': 'short',
},
},
fix: true,
})
.then((result) => {
expect(result.output).toBe(code);
});
});
});

describe('fix with BOM', () => {
Expand Down