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 false negatives on second run for cache and severity option #6384

Merged
merged 10 commits into from Oct 6, 2022

Conversation

kimulaco
Copy link
Member

@kimulaco kimulaco commented Oct 2, 2022

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

Closes #4715

Is there anything in the PR that needs further explanation?

No, it's self-explanatory.

@changeset-bot
Copy link

changeset-bot bot commented Oct 2, 2022

🦋 Changeset detected

Latest commit: a85f2e1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
stylelint Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jeddy3 jeddy3 changed the title Cache PostcssResult Fix false negatives on second run for cache and plugins Oct 3, 2022
@ybiquitous
Copy link
Member

@kimulaco Thanks for creating the PR.

I am looking into this bug. So, the bug seems related to not plugins but severity. See the code below.

If stylelintError is true, a cache is discarded here: ⬇️

stylelint/lib/standalone.js

Lines 219 to 222 in 751acda

if (postcssResult.stylelint.stylelintError && useCache) {
debug(`${absoluteFilepath} contains linting errors and will not be cached.`);
stylelint._fileCache.removeEntry(absoluteFilepath);
}

Then, stylelintError is set to true here (only if the severity is error): ⬇️

if (!result.stylelint.stylelintError && severity === 'error') {
result.stylelint.stylelintError = true;
}

The following is a production: ⬇️

$ cat a.css
a { colo: red; }

$ cat package.json
{
  "dependencies": {
    "stylelint": "14.13.0"
  },
  "stylelint": {
    "rules": {
      "property-no-unknown": [true, { "severity": "warning" }]
    }
  }
}

$ rm -f .stylelintcache

$ npx stylelint *.css --cache -f json | jq -c '.[0].warnings'
[{"line":1,"column":5,"endLine":1,"endColumn":9,"rule":"property-no-unknown","severity":"warning","text":"Unexpected unknown property \"colo\" (property-no-unknown)"}]

$ npx stylelint *.css --cache -f json | jq -c '.[0].warnings'
[]

We can see the second run emits no warnings.

On the other hand, the second run emits warnings after changing the severity from warning to error in the package.json file above.

$ npx stylelint *.css --cache -f json | jq -c '.[0].warnings'
[{"line":1,"column":5,"endLine":1,"endColumn":9,"rule":"property-no-unknown","severity":"error","text":"Unexpected unknown property \"colo\" (property-no-unknown)"}]

$ npx stylelint *.css --cache -f json | jq -c '.[0].warnings'
[{"line":1,"column":5,"endLine":1,"endColumn":9,"rule":"property-no-unknown","severity":"error","text":"Unexpected unknown property \"colo\" (property-no-unknown)"}]

@kimulaco
Copy link
Member Author

kimulaco commented Oct 4, 2022

@ybiquitous Thanks. I thought I needed to cache the result. But I could understand that it was caused by a condition to remove the cache.
Please give me some time to fix it. 🙏

@ybiquitous
Copy link
Member

@kimulaco I hope you don't mind. I appreciate your contribution to this project. 😊

@jeddy3 jeddy3 changed the title Fix false negatives on second run for cache and plugins Fix false negatives on second run for cache and severity option Oct 5, 2022
@kimulaco
Copy link
Member Author

kimulaco commented Oct 5, 2022

@ybiquitous I fixed. It contains a new approach, so I would like your opinion.

First, I reverted all codes that cache the postcssResult. (aa28b1a, db19002)

Then, I added stylelintWarning to StylelintPostcssResult. The cache is discarded also when stylelintWarning is true.
Because, stylelintError is equal to errored, so to prevent breaking spec of errored.

### `errored`
Boolean. If `true`, at least one rule with an "error"-level severity registered a problem.

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

@kimulaco Thank you, LGTM! 👍🏼

In addition, could you please add a changelog entry? The entry line should be like this (from the PR title): ⬇️

Fixed: false negatives on second run for cache and `severity` option

@kimulaco
Copy link
Member Author

kimulaco commented Oct 5, 2022

@ybiquitous Thanks for the review! I added a changelog. a85f2e1

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM 👍🏼

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@jeddy3 jeddy3 merged commit 168b71a into stylelint:main Oct 6, 2022
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.

Fix false negatives on second run for cache and plugins
3 participants