Preserve .eslintcache on fixes
#898
Conversation
|
Now that I think about this a little more, we should probably explore using Also, I guess the Appveyor spec failure is the same race condition failure that caused us to disable the other test in that suite. Maybe breaking them into separate describe blocks will help… I'll try it later. |
|
That's a bizarre failure in the AppVeyor build, it looks like it grabbed the notification for one of the debug tooltip tests, I'm not entirely sure we can actually fix that within the limitations of Jasmine 1.3 that Atom uses 😕. |
src/main.js
Outdated
| // Abort for invalid or unsaved text editors | ||
| atom.notifications.addError('Linter-ESLint: Please save before fixing') |
There was a problem hiding this comment.
Note, we can now perform fixes on unsaved text!
There was a problem hiding this comment.
There is a small caveat here. If you make a change to introduce a linting error within a previously-passing file without saving, and the fix results in the same code as what was previously saved, manually running a fix command will have no visible effect in the editor. That's because the file on disk will essentially not be changed, and so Atom won't know to reload the editor. I think that's a small enough edge case that we don't need to worry about it, but wanted to make you aware.
IanVS
force-pushed
the
issue635-preserve-cache
branch
2 times, most recently
from
68993c0 to
1fff6c8
Compare
IanVS
force-pushed
the
issue635-preserve-cache
branch
from
1fff6c8 to
72c0f1d
Compare
spec/linter-eslint-spec.js
Outdated
| fs.createReadStream(fileToCopyPath).pipe(ws) | ||
| }) | ||
| } | ||
|
|
||
| function copyFileToTempDir(fileToCopyPath) { |
src/worker.js
Outdated
| @@ -36,8 +36,8 @@ function lintJob({ cliEngineOptions, contents, eslint, filePath }) { | |||
| : cliEngine.executeOnFiles([filePath]) | |||
There was a problem hiding this comment.
Closest I can get to commenting while still be in the PR:
contents will always be a string now, so this entire check is irrelevant now.
IanVS
force-pushed
the
issue635-preserve-cache
branch
from
e1e158a to
62f416f
Compare
|
@Arcanemagus thanks for the review. I've addressed your feedback and squashed commits. |
|
Currently although we can get the list of fixes for an unsaved buffer, since we are using |
|
Hah! I spend a few minutes in your non-compiling branch, and now I'm already forgetting to check in compiled files! |
IanVS
force-pushed
the
issue635-preserve-cache
branch
from
34008d2 to
862dcc1
Compare
There’s no need for it to also handle opening the file. We may not always want to copy and then also immediately open it.
IanVS
force-pushed
the
issue635-preserve-cache
branch
from
862dcc1 to
4ff4b82
Compare
This prevents ESLint from deleting the users cache file, because `executeOnText` bypasses the cache logic in ESLint entirely.
IanVS
force-pushed
the
issue635-preserve-cache
branch
from
4ff4b82 to
580e569
Compare
Fixes #635
The main point of this PR is to avoid deleting the
.eslintcachefile when performing fix jobs inlinter-eslint. Previously, they were deleted because if ESLint is run without thecacheoption, it will automatically delete any existing cache files.linter-eslintdoesn't delete the cache on regularlintjobs because it usesexecuteOnText, which bypasses the cache and does not delete it.Update: I've changed this PR to use
executeOnTextfor fix jobs as suggested by @not-an-aardvark in #873 (comment). It's a much better solution than my first approach.The approach I took was fairly simplistic. We will search for a.eslintcachefile, and if we find it, we will setcachefor fix jobs. There is a slight danger here that we may create extra.eslintcachefiles in unexpected locations, because that setting will also create cache files if none exist. In my own testing so far I haven't experienced that problem, but we should be on the lookout for it in more complicated project structures.At this time I think it's reasonable to only consider files named.eslintcache, as that is the default and most common. If there is overwhelming demand for a config setting, we can consider that, but for now I'd like to keep it as simple as possible.