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

New: Implement cacheStrategy (refs eslint/rfcs#63) #14119

Merged
merged 13 commits into from Feb 26, 2021

Conversation

chambo-e
Copy link
Contributor

@chambo-e chambo-e commented Feb 16, 2021

Hello 👋

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[X] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

This is a follow up of #13713 which seems abandoned.

I corrected the tests by binding every stubs to the newly created sinon sandbox

Is there anything you'd like reviewers to focus on?

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Feb 16, 2021
@eslint-deprecated
Copy link

Hi @chambo-e!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The issue reference must be formatted as follows:

    If the pull request addresses an issue, then the issue number should be mentioned at the end. If the commit doesn't completely fix the issue, then use (refs #1234) instead of (fixes #1234).

    Here are some good commit message summary examples:

    Build: Update Travis to only test Node 0.10 (refs #734)
    Fix: Semi rule incorrectly flagging extra semicolon (fixes #840)
    Upgrade: Esprima to 1.2, switch to using comment attachment (fixes #730)
    

Read more about contributing to ESLint here

1 similar comment
@eslint-deprecated
Copy link

Hi @chambo-e!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The issue reference must be formatted as follows:

    If the pull request addresses an issue, then the issue number should be mentioned at the end. If the commit doesn't completely fix the issue, then use (refs #1234) instead of (fixes #1234).

    Here are some good commit message summary examples:

    Build: Update Travis to only test Node 0.10 (refs #734)
    Fix: Semi rule incorrectly flagging extra semicolon (fixes #840)
    Upgrade: Esprima to 1.2, switch to using comment attachment (fixes #730)
    

Read more about contributing to ESLint here

@eslint-deprecated
Copy link

Hi @chambo-e!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The issue reference must be formatted as follows:

    If the pull request addresses an issue, then the issue number should be mentioned at the end. If the commit doesn't completely fix the issue, then use (refs #1234) instead of (fixes #1234).

    Here are some good commit message summary examples:

    Build: Update Travis to only test Node 0.10 (refs #734)
    Fix: Semi rule incorrectly flagging extra semicolon (fixes #840)
    Upgrade: Esprima to 1.2, switch to using comment attachment (fixes #730)
    

Read more about contributing to ESLint here

@chambo-e chambo-e changed the title New: implement eslint/rfcs#63 New: implement (refs eslint/rfcs#63) Feb 16, 2021
@eslint-deprecated
Copy link

Hi @chambo-e!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The issue reference must be formatted as follows:

    If the pull request addresses an issue, then the issue number should be mentioned at the end. If the commit doesn't completely fix the issue, then use (refs #1234) instead of (fixes #1234).

    Here are some good commit message summary examples:

    Build: Update Travis to only test Node 0.10 (refs #734)
    Fix: Semi rule incorrectly flagging extra semicolon (fixes #840)
    Upgrade: Esprima to 1.2, switch to using comment attachment (fixes #730)
    

Read more about contributing to ESLint here

@chambo-e chambo-e changed the title New: implement (refs eslint/rfcs#63) New: Implement cacheStrategy (refs eslint/rfcs#63) Feb 16, 2021
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Overall looks good, just a couple of things to clarify.

docs/user-guide/command-line-interface.md Outdated Show resolved Hide resolved
lib/cli-engine/lint-result-cache.js Outdated Show resolved Hide resolved
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion cli Relates to ESLint's command-line interface core Relates to ESLint's core APIs and features feature This change adds a new feature to ESLint and removed triage An ESLint team member will look at this issue soon labels Feb 18, 2021
Copy link
Contributor

@royriojas royriojas left a comment

Choose a reason for hiding this comment

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

Very nice

@chambo-e chambo-e requested a review from nzakas February 18, 2021 13:48
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. I'd like another set of eyes on this before merging.

lib/eslint/eslint.js Show resolved Hide resolved
tests/lib/cli-engine/cli-engine.js Show resolved Hide resolved
lib/cli-engine/lint-result-cache.js Outdated Show resolved Hide resolved
lib/eslint/eslint.js Show resolved Hide resolved
lib/cli-engine/lint-result-cache.js Outdated Show resolved Hide resolved
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

It would be good to also fix the behavior in the case that is described here:

eslint/rfcs#63 (comment)

Edit: Not in this PR.

@nzakas
Copy link
Member

nzakas commented Feb 19, 2021

@mdjermanovic you indicated in the RFC that this wasn't a blocker. Is that something we can manage via a separate issue rather than blocking this PR for it?

@mdjermanovic
Copy link
Member

Agreed we shouldn't address that in this PR. The issue might get fixed in file-entry-cache (jaredwray/file-entry-cache#22).

I meant it shouldn't be a blocker for this feature. If it isn't fixable, we can tell users to always delete the cache file before changing the strategy.

@royriojas
Copy link
Contributor

@mdjermanovic @nzakas published file-entry-cache@6.0.1 with @mdjermanovic fix

@mdjermanovic
Copy link
Member

@royriojas thanks!

@chambo-e can you update file-entry-cache to ^6.0.1 in package.json?

This was referenced Mar 17, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Aug 26, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Aug 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion cli Relates to ESLint's command-line interface core Relates to ESLint's core APIs and features feature This change adds a new feature to ESLint upgrade This change is related to a dependency upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants