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

Stop skipping linting when --report-needless-disables CLI flag is used. Fix #4076 #4151

Conversation

thibaudcolas
Copy link
Member

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

Fixes #4076, #2369, #4100.

Is there anything in the PR that needs further explanation?

This PR does the same change as #2731, which was closed for cleanup and never reopened. The current behavior can be a source of confusion for people used to ESLint’s --report-needless-disables. It also seems a bit wasteful to have to run linting twice just to get the extra reporting of unused disable comments.

Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

I like that you went extra mile and added tests!

@hudochenkov
Copy link
Member

hudochenkov commented Jul 19, 2019

@evilebottnawi do you think it's a breaking change or not? I think it's a breaking change.

@alexander-akait
Copy link
Member

@hudochenkov And yes and no, for me it is looks like bug (eslint reports erroneous comments and problems with the code together), but some developers potentially could use this command as separate

Will be great hear other opinion

@thibaudcolas
Copy link
Member Author

thibaudcolas commented Jul 19, 2019

This change looks like the following examples from the project’s semantic versioning policy:

  • minor release (might break your lint build)
    • a bug fix in a rule that results in stylelint reporting more errors
    • a new CLI capability is created
  • major release (likely to break your lint build)
    • a change in the documented behaviour of an existing rule results in stylelint reporting more errors by default
    • part of the a CLI is removed or changed in an incompatible way

To me it feels like if the change is considered to be a bug fix then it could be done as a minor release (and if there is breakage so be it), while if it’s considered to be a change in documented behavior of this CLI flag then it’s probably for a major release. A patch release seems inappropriate in any case since there is some potential for this to break people’s linting.

@alexander-akait
Copy link
Member

/cc @jeddy3 what do you think about this? For me it is looks like a big bug

@jeddy3
Copy link
Member

jeddy3 commented Jul 24, 2019

/cc @jeddy3 what do you think about this? For me it is looks like a big bug

To me it feels like if the change is considered to be a bug fix then it could be done as a minor release (and if there is breakage so be it), while if it’s considered to be a change in documented behavior of this CLI flag then it’s probably for a major release.

I think the behaviour in this PR is probably what users expect is happening and the original doc ("Report stylelint-disable comments that are not blocking a lint warning.") can be read that way too. So, minor release?

@alexander-akait
Copy link
Member

@jeddy3 👍

@hudochenkov hudochenkov merged commit 2c43e45 into stylelint:master Jul 31, 2019
@hudochenkov
Copy link
Member

  • Changed: --report-needless-disables CLI flag now reports needless disables and runs linting (#4151).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Report Needless Disables with Rule Errors and Warnings
4 participants