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 regression where report flags don't exit with non-zero #5046

Closed
XhmikosR opened this issue Nov 20, 2020 · 4 comments · Fixed by #5079
Closed

Fix regression where report flags don't exit with non-zero #5046

XhmikosR opened this issue Nov 20, 2020 · 4 comments · Fixed by #5079
Labels
help wanted is likely non-trival and help is wanted status: ready to implement is ready to be worked on by someone type: bug a problem with a feature or rule

Comments

@XhmikosR
Copy link
Member

Clearly describe the bug

When a needless disable is found, the process exits with a zero code.

What stylelint configuration is needed to reproduce the bug?

{
  "extends": [
    "stylelint-config-twbs-bootstrap/scss"
  ],
  "rules": {
    "function-disallowed-list": [
      "calc",
      "lighten",
      "darken"
    ],
    "property-disallowed-list": [
      "border-radius",
      "border-top-left-radius",
      "border-top-right-radius",
      "border-bottom-right-radius",
      "border-bottom-left-radius",
      "transition"
    ],
    "scss/dollar-variable-default": [
      true,
      {
        "ignore": "local"
      }
    ],
    "scss/selector-no-union-class-name": true
  }
}

Which version of stylelint are you using?

13.8.0

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

CLI: stylelint "**/*.{css,scss}" --cache --cache-location .cache/.stylelintcache --rd

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

Probably not?

What did you expect to happen?

The process should error out and exit with non-zero code

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

The needless disables are printed, but the process exits with 0.

scss/_reboot.scss
 1:1  ×  Needless disable for "at-rule-no-vendor-prefix"  --report-needless-disables

Test repo:

  1. git clone https://github.com/twbs/bootstrap.git
  2. npm i && npm run css-lint-stylelint

Notes:

  1. Running the same command one more time, does not print the needless disables at all unless I delete the cache file.
  2. Those needless disables weren't caught before 13.8.0, I think. But when a needless disable was found, the process exited with non-zero code in the previous Stylelint version. So, it looks like this is a regression in 13.8.0.
@jeddy3
Copy link
Member

jeddy3 commented Nov 20, 2020

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

Yes, it appears to be a regression as the:

  • CLI should exit with non-zero code
  • Node.js API should set the errored property to true

When any of the report* flags report a violation.

I've labelled the issue as ready to implement. If anyone has time to look into this then please consider contributing.

Running the same command one more time, does not print the needless disables at all unless I delete the cache file.

That doesn't sound good! We should create another issue to investigate that.


Ref: #4896 (comment)

@jeddy3 jeddy3 changed the title --rd doesn't exit with non-zero Fix regression where report flags don't exit with non-zero Nov 20, 2020
@jeddy3 jeddy3 added help wanted is likely non-trival and help is wanted status: ready to implement is ready to be worked on by someone type: bug a problem with a feature or rule labels Nov 20, 2020
@XhmikosR
Copy link
Member Author

XhmikosR commented Nov 20, 2020

That doesn't sound good! We should create another issue to investigate that.

I'm not sure if it's a separate issue (although it sounds like it), I'll need to look into it more unless someone beats me to it :)

EDIT: indeed it happens, so I made #5049

@jeddy3
Copy link
Member

jeddy3 commented Nov 20, 2020

Possible fix detailed in #4896 (comment).

@edg2s
Copy link

edg2s commented Nov 22, 2020

When I originally reported this, I was expecting warnings from the Node API, as that is what the equivalent feature in ESLint does: https://eslint.org/docs/user-guide/configuring#report-unused-eslint-disable-comments

When moving the feature to the Node API they made this a warning to allow more rule fixes to be considered patch releases: eslint/eslint#12703 (comment)

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 status: ready to implement is ready to be worked on by someone type: bug a problem with a feature or rule
Development

Successfully merging a pull request may close this issue.

3 participants