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 autofix introducing new violations #3853

Open
wKich opened this issue Dec 16, 2018 · 7 comments
Open

Fix autofix introducing new violations #3853

wKich opened this issue Dec 16, 2018 · 7 comments
Labels
help wanted is likely non-trival and help is wanted priority: high is impactful on users status: ask to implement ask before implementing as may no longer be relevant type: bug a problem with a feature or rule

Comments

@wKich
Copy link

wKich commented Dec 16, 2018

Clearly describe the bug

Some rule ignore indentation rule and as result after auto fix I receive error about indentation and should call auto fix second time to fix my files. If I try to use prettier-stylelint than I get unfixable indentation error, because stylelint is called once.

Which rule, if any, is the bug related to?

declaration-colon-newline-after or maybe value-list-comma-newline-after. I think this happen because of PR #3588

What LESS is needed to reproduce the bug?

:local {
  .root {
    &:active .box {
      background: #e1e1e1;
      box-shadow: 0 -1px 0 0 rgba(0, 0, 0, 0.1), 0 0 0 1px rgba(0, 0, 0, 0.2),
        inset 0 1px 2px 0 rgba(0, 0, 0, 0.1);
    }
  }
}

What stylelint configuration is needed to reproduce the bug?

extends:
  - stylelint-config-standard
  - stylelint-config-css-modules

Which version of stylelint are you using?

    "stylelint": "^9.9.0",
    "stylelint-config-css-modules": "^1.3.0",
    "stylelint-config-standard": "^18.2.0",

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

CLI with stylelint "./**/*.less" --fix

Does the bug relate to non-standard syntax (e.g. SCSS, Less etc.)?

I think No.

What did you expect to happen?

Correctly formatted file:

:local {
  .root {
    &:active .box {
      background: #e1e1e1;
      box-shadow:
        0 -1px 0 0 rgba(0, 0, 0, 0.1),
        0 0 0 1px rgba(0, 0, 0, 0.2),
        inset 0 1px 2px 0 rgba(0, 0, 0, 0.1);
    }
  }
}

What actually happened (e.g. what warnings or errors did you get)?

After first auto fix I receive:

:local {
  .root {
    &:active .box {
      background: #e1e1e1;
      box-shadow:
        0 -1px 0 0 rgba(0, 0, 0, 0.1),
 0 0 0 1px rgba(0, 0, 0, 0.2),        /* <--- Here line with indentation error */
        inset 0 1px 2px 0 rgba(0, 0, 0, 0.1);
    }
  }
}
@jeddy3
Copy link
Member

jeddy3 commented Dec 17, 2018

@wKich Thanks for the clear report and for using the template.

I receive error about indentation and should call auto fix second time to fix my files

This is, unfortunately, a known limitation. We don't yet have a robust way to avoid introducing violations for rules that have already been run.

As a workaround, you can see if adding the indentation rule as the last entry in your configuration object helps? e.g.

extends:
  - stylelint-config-standard
  - stylelint-config-css-modules
rules:
  - indentation
    - 2 

There is some further discussion on this limitation, but help is needed to resolve it in a robust way.

I'll keep this issue open, but rename it so that we can draw attention to this limitation and hopefully galvantise support to fix it.

@jeddy3 jeddy3 added status: ready to implement is ready to be worked on by someone type: bug a problem with a feature or rule labels Dec 17, 2018
@jeddy3 jeddy3 changed the title auto fix for declaration-colon-newline-after lead to another issue Fix autofix introducing new violations Dec 17, 2018
@golopot
Copy link

golopot commented Mar 26, 2019

I think stylelint cli should run fixer multiple times, like what eslint does.

After applying fixes, ESLint will run all of the enabled rules again on the fixed code, potentially applying more fixes. This process will repeat up to 10 times, or until no more fixable problems are found. Afterwards, any remaining problems will be reported as usual.

@jeddy3
Copy link
Member

jeddy3 commented Apr 2, 2019

I think stylelint cli should run fixer multiple times, like what eslint does.

SGTM. Feel free to contribute a proof of concept PR.

@Mouvedia
Copy link
Member

Mouvedia commented Aug 11, 2022

Is this about rolling back to the state before the last fix—and skip that error—if a new error has been detected or is it about introducing a new flag (e.g. --maximum-run-per-file)?
These two features are completely different.

PRO of the former: you only have to run it a second time at the end (to rollback specific hunk of code)
CONS of the former:

  • you have to fix the skipped errors manually
  • the rollback implementation is pretty complicated

CONS of the latter:

  • run speed will greatly diminish (running it again to verify no errors were introduced for each file is overkill)
  • risk of infinite loops

@jeddy3
Copy link
Member

jeddy3 commented Aug 18, 2022

Is this about rolling back to the state before the last fix—and skip that error—if a new error has been detected or is it about introducing a new flag (e.g. --maximum-run-per-file)?

The latter. Although, it's less of an issue these days now that we're moving away from stylistic rules as the rules that avoid errors and enforce convention tend not to introduce new violations.

@Mouvedia
Copy link
Member

Mouvedia commented Aug 18, 2022

the rules that avoid errors and enforce convention tend not to introduce new violations

  • indentation
    • value-list-comma-newline-after
    • function-comma-newline-after
    • declaration-colon-newline-after
    • declaration-block-semicolon-newline-after
    • block-opening-brace-newline-after
    • block-closing-brace-newline-after
    • selector-list-comma-newline-after
    • media-query-list-comma-newline-after
    • at-rule-name-newline-after
    • at-rule-semicolon-newline-after

Are all the *-newline-after rules affected?
Can we share a common util for the handling of indentation?
related discussion: #2532 (comment)
related question: stylelint/stylelint-config-standard@090055e#r81509647

Is selector-not-notation the only rule that may have an impact on no-descending-specificity?
ref: #6236
Are there rules that affect selector-max-specificity?
ref 2f24d8b

I am asking all of this for the projected tests.

Copy link
Contributor

This issue is older than one month. Please ask before opening a pull request, as it may no longer be relevant.

@github-actions github-actions bot added status: ask to implement ask before implementing as may no longer be relevant and removed status: ready to implement is ready to be worked on by someone labels Jan 22, 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: ask to implement ask before implementing as may no longer be relevant type: bug a problem with a feature or rule
Development

No branches or pull requests

4 participants