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

Add stylelint-disable commands support to autofix #2643

Closed
39 tasks done
dan-gamble opened this issue Jun 15, 2017 · 34 comments · Fixed by #7895
Closed
39 tasks done

Add stylelint-disable commands support to autofix #2643

dan-gamble opened this issue Jun 15, 2017 · 34 comments · Fixed by #7895
Labels
help wanted is likely non-trival and help is wanted priority: high is impactful on users status: wip is being worked on by someone type: umbrella a group of related issues

Comments

@dan-gamble
Copy link
Contributor

dan-gamble commented Jun 15, 2017

Describe the issue. Is it a bug or a feature request (new rule, new option, etc.)?

I have a file that has /* stylelint-disable */ /* stylelint-enable */ wrapped around it but when using --fix it tries to format this file and breaks it quite bad.

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

--fix

What CSS is needed to reproduce this issue?

Before
After

A few of the issues:

  • Double ;; on L38
  • No ; on L92 (I imagine this is where the ;; comes from

What stylelint configuration is needed to reproduce this issue?

Config

The config is pretty huge but i don't think it's the problem here

Which version of stylelint are you using?

7.11.0

How are you running stylelint: CLI, PostCSS plugin, Node API?

CLI through lint-staged


  • recommended config
    • declaration-block-no-duplicate-properties
    • function-calc-no-unspaced-operator
  • standard config
    • alpha-value-notation
    • at-rule-empty-line-before
    • at-rule-no-vendor-prefix
    • color-function-notation
    • color-hex-length
    • comment-empty-line-before
    • comment-whitespace-inside
    • custom-property-empty-line-before
    • declaration-block-no-redundant-longhand-properties
    • declaration-empty-line-before
    • font-family-name-quotes
    • function-name-case
    • function-url-quotes
    • hue-degree-notation
    • import-notation
    • keyframe-selector-notation
    • length-zero-no-unit
    • lightness-notation
    • media-feature-range-notation
    • media-feature-name-no-vendor-prefix
    • property-no-vendor-prefix
    • rule-empty-line-before
    • selector-attribute-quotes
    • selector-no-vendor-prefix
    • selector-not-notation
    • selector-pseudo-element-colon-notation
    • selector-type-case
    • shorthand-property-no-redundant-values
    • value-keyword-case
    • value-no-vendor-prefix
  • font-weight-notation

  • context.fix warning
    • no-empty-source
  • documentation
    • rules#add-autofix
    • plugins#the-anatomy-of-a-plugin
    • options#fix
@alexander-akait alexander-akait added status: ready to implement is ready to be worked on by someone type: bug a problem with a feature or rule labels Jun 15, 2017
@alexander-akait
Copy link
Member

I think it is bug, should be respected by default.

@dan-gamble
Copy link
Contributor Author

I think it also ignore ignoreFiles too fwiw.

@hudochenkov
Copy link
Member

I know why stylelint-disable doesn't work with --fix. stylelint creates disabledRanges while processing style sheet. While linting, stylelint doesn't care about this ranges. They only matter, when stylelint generates report. It compares each violation and deletes violations within disabledRange.

I have no idea what to do with --fix and disabled ranges yet :(

@alexander-akait
Copy link
Member

@hudochenkov maybe create util isStylelintDisable and check on this before fix in rule?

@jeddy3 jeddy3 added type: enhancement a new feature that isn't related to rules and removed type: bug a problem with a feature or rule labels Jun 20, 2017
@jeddy3 jeddy3 changed the title Have autofix respect stylelint-disable Autofix should respect stylelint-disable commands Jun 20, 2017
@hudochenkov
Copy link
Member

I came up to another problem. Disabled ranges assigned before file linted/fixed. For rules which altered lines (change empty lines, add line endings), disabled ranges might be incorrect after few fixes. Even after fixes within one rule.

This problem affects every rule. Because if user has rule with fixing which changes lines, then every rule after this rule will have incorrect disabled ranges.

@jeddy3
Copy link
Member

jeddy3 commented Jun 20, 2017

@hudochenkov Thanks for digging deeper. I suspect there are going to be some significant hurdles to overcome with this issue. As such, I suggest we push on with 8.0.0 and revisit this later. The autofix feature is marked as experimental and support for disable-ranges can be seen as a feature we'd like add, rather than a showstopping bug.

@hudochenkov
Copy link
Member

I suspect there are going to be some significant hurdles to overcome with this issue. As such, I suggest we push on with 8.0.0 and revisit this later.

Agree. We should not close this issue, to not forget about this problem.

The autofix feature is marked as experimental and support for disable-ranges can be seen as a feature we'd like add, rather than a showstopping bug.

I think we should add a note somewhere about this caveat. And suggested strategy: if user is using stylelint --fix and stylelint-disable* comments, then stylelint should be run twice. First run there might be false violations (or ignored violations), because of shifted disable ranges. On second run all results would be correct. Also worth adding, that this strategy is for correct violation reports. --fix will fix everything regardless disabled ranges.

@alexander-akait
Copy link
Member

@hudochenkov can we learn how it does eslint?

@hudochenkov
Copy link
Member

@evilebottnawi of course we can. But I have a feeling that it won't help us, because of differences in architecture and approaches. Also I remember something from discussions, when we were discussing how to do autofixing. ESLint make multiple runs until everything is fixed and violations verified. And it also has some ranges for autofixing or something, that recalculated constantly. I might be mistaken about the last one.

@zkuzmic

This comment was marked as outdated.

@jeddy3

This comment was marked as outdated.

@zkuzmic

This comment was marked as outdated.

chmanie pushed a commit to JoinColony/colonyDapp that referenced this issue Oct 18, 2018
Stylelint also fix-breaks calc() functions as it removes units of values that need it (see stylelint/stylelint#3037). It also auto-fixes it (and hence breaks it then): stylelint/stylelint#2643, so we have to disable the whole rule for now. In order to be able to add comments to the stylelint config I moved it to a .js file.
@ybiquitous
Copy link
Member

I think it's better to work on documenting now because we may release this new fix option soon.

@Mouvedia Mouvedia removed the good first issue is good for newcomers label Jun 24, 2024
@ybiquitous
Copy link
Member

ybiquitous commented Jul 11, 2024

Refactoring all the rules is done. ✅

@Mouvedia
Copy link
Member

Mouvedia commented Aug 6, 2024

FYI

@ybiquitous ybiquitous unpinned this issue Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted is likely non-trival and help is wanted priority: high is impactful on users status: wip is being worked on by someone type: umbrella a group of related issues
Development

Successfully merging a pull request may close this issue.