Chore: Extract lint result cache logic (refs #9948) #10562
Conversation
eslint-deprecated
bot
added
the
triage
platinumazure
added
cli
tests/lib/util/lint-result-cache.js
Outdated
| globInputPaths: false | ||
| }); | ||
|
|
||
| // fakeSuccessResults = cliEngine.executeOnFiles([path.join(fixturePath, "test-no-errors.js")]).results[0]; |
There was a problem hiding this comment.
Should this comment be removed?
tests/lib/util/lint-result-cache.js
Outdated
| fakeConfigHelper, | ||
| fakeErrorResults; | ||
|
|
||
| // let fakeSuccessResults; |
There was a problem hiding this comment.
Should this comment be removed?
lib/util/lint-result-cache.js
Outdated
| return null; | ||
| } | ||
|
|
||
| // TODO (platinumazure): Hydrate any missing elements from results |
There was a problem hiding this comment.
I'm not sure I understand the implications of this comment -- is there a correctness issue for missing elements?
There was a problem hiding this comment.
No, there's no correctness issue for now. This is a reference to where I would need to add code to implement the feature requested in #9984. I will remove this comment.
|
I'll rebase and then push a commit to remove the comments called out in review. Thanks! |
platinumazure
force-pushed
the
lint-result-cache
branch
from
4b2e168
to
5501ea6
Compare
|
@not-an-aardvark Friendly ping: Have I addressed your concerns? |
| @@ -0,0 +1 @@ | |||
| console.log("Hello, world!"); | |||
There was a problem hiding this comment.
I should remove this file.
|
Sorry about the delay -- I haven't had time to look at this again, but my concerns were relatively minor, so feel free to merge if you think you've addressed them. |
lib/cli-engine.js
Outdated
| @@ -434,6 +419,17 @@ class CLIEngine { | |||
| } | |||
|
|
|||
| this.config = new Config(this.options, this.linter); | |||
|
|
|||
| if (options.cache) { | |||
There was a problem hiding this comment.
Nit: Should this maybe be this.options.cache for consistency?
|
Adding "do not merge" to avoid early merge while I rebase, delete an unnecessary file, and implement suggestions provided in feedback. |
platinumazure
removed
the
do not merge
|
I've addressed all feedback and the Travis/Appveyor builds passed, so I'm going to merge. Thanks to everyone who reviewed for your feedback! |
eslint-deprecated
bot
added
the
archived due to age
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain: Extract lint result cache logic to its own utility module.
Motivation: This is preparation for a possible implementation of #9948, which might require that the object we cache for linted files will no longer have a 1-to-1 correspondence with the lint result object.
What changes did you make? (Give an overview)
Extracted cache logic for linting results (only) into its own file. The behavior should be unchanged.
Is there anything you'd like reviewers to focus on?
I'm not happy with the way I've set up mocks etc. in the tests. It's not immediately obvious to future contributors why some setups are in an inner
beforeEachvs an outer or middlebeforeand why more of the setups couldn't be closer together, but I couldn't figure out a great way to improve upon what I currently have. Any tips for making those more organized/easier to read (possibly with a different library)?