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

Report disable errors the same way as lint errors #4896

Closed
nex3 opened this issue Aug 13, 2020 · 8 comments · Fixed by #4973
Closed

Report disable errors the same way as lint errors #4896

nex3 opened this issue Aug 13, 2020 · 8 comments · Fixed by #4973
Labels
status: ready to implement is ready to be worked on by someone type: enhancement a new feature that isn't related to rules

Comments

@nex3
Copy link
Contributor

nex3 commented Aug 13, 2020

What is the problem you're trying to solve?

Currently, errors caused by reportNeedlessDisables, reportInvalidScopeDisables, and eventually reportUnscopedDisables (#2292) and reportDisables (#4875) are reported in a format that's inconsistent with (and less visually appealing than) the way normal lints are recorded.

What solution would you like to see?

I propose that these disable issues instead be converted to the same object structure as lints, and be passed to the standard lint formatter to be reported appropriately. That way they'll automatically play nicely with both default and custom formatters, and look consistent with the most common reports.

@jeddy3 jeddy3 added status: ready to implement is ready to be worked on by someone type: enhancement a new feature that isn't related to rules labels Aug 31, 2020
@jeddy3
Copy link
Member

jeddy3 commented Aug 31, 2020

Sounds good to me. I don't know why we didn't do that originally.

nex3 added a commit that referenced this issue Sep 25, 2020
nex3 added a commit that referenced this issue Oct 9, 2020
Rather than returning disables that are considered "invalid" for
various reasons (such as --report-needless-disables) as a separate
list of objects that formatters must handle individually, stylelint
now converts them into standard lint warnings. This allows formatters
to automatically format them the same as normal warnings without any
extra disable-specific code.

Closes #4896
nex3 added a commit that referenced this issue Oct 14, 2020
Rather than returning disables that are considered "invalid" for
various reasons (such as --report-needless-disables) as a separate
list of objects that formatters must handle individually, stylelint
now converts them into standard lint warnings. This allows formatters
to automatically format them the same as normal warnings without any
extra disable-specific code.

Closes #4896
@edg2s
Copy link

edg2s commented Nov 19, 2020

Is there a reason these are styled with red x's in the output, instead of yellow warning triangles as is used for other warnings?

image

@nex3
Copy link
Contributor Author

nex3 commented Nov 19, 2020

Stylelint makes the distinction between "warnings" (displayed with yellow triangles) and "errors" (displayed with red Xs). Normally, lints have a way of configuring which type they'll be reported as, but since these aren't configurable in the same way we had to make a choice—I chose errors, to match the idea that failing one of these requirements would cause the linter to produce a non-zero exit code.

@edg2s
Copy link

edg2s commented Nov 19, 2020

I get a 0 exit code with these in the command line:

$ ./node_modules/stylelint/bin/stylelint.js --rd src/styles/widgets/SelectFileWidget.less 

src/styles/widgets/SelectFileWidget.less
 21:4  ✖  Needless disable for "plugin/no-unsupported-browser-features"  --report-needless-disables

$ echo $?
0

Similarly, using the Node API, the "errored" flag is not set, and these are marked as warnings.

@nex3
Copy link
Contributor Author

nex3 commented Nov 19, 2020

I'm surprised by that, but I'm afraid you've reached the limit of my stylelint expertise 🤷‍♀️. Maybe one of the more experienced maintainers could chime in?

@XhmikosR
Copy link
Member

I opened #5046 this morning for the wrong error code.

@vankop
Copy link
Member

vankop commented Nov 20, 2020

As I see regression is here #4973

We need to hit this condition

const errored = stylelintResults.some(

for
https://github.com/stylelint/stylelint/blob/master/lib/cli.js#L563

feel free to send a PR

@vankop vankop reopened this Nov 20, 2020
@jeddy3
Copy link
Member

jeddy3 commented Nov 20, 2020

@vankop Thanks for identifying a possible fix!

Let's track the regression in @XhmikosR new issue: #5046

@jeddy3 jeddy3 closed this as completed Nov 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready to implement is ready to be worked on by someone type: enhancement a new feature that isn't related to rules
Development

Successfully merging a pull request may close this issue.

5 participants