-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
avoid pre-reading files #5973
avoid pre-reading files #5973
Conversation
Here's an example of your CHANGELOG entry: * avoid pre-reading files.
[OneSadCookie](https://github.com/OneSadCookie)
[#issue_number](https://github.com/realm/SwiftLint/issues/issue_number) note: There are two invisible spaces after the entry's text. Generated by 🚫 Danger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically looks good to me!
Would be great to have a test for linting and correcting empty files with (mock) rules that do/don't support this.
86e8f3b
to
bde693d
Compare
bde693d
to
54bd5bc
Compare
I've tried to write those tests, but pretty low confidence that that's "the right way"; pointers appreciated. I tried to see if there was a way to write an integration test that the files' contents aren't read when the cache is populated, but short of modifying SourceKitten to report contents reads, or magical unix shenanigans (I think you can somehow get a pipe at a path so it gets opened like a regular file, maybe you could tell if data was consumed on the pipe?), I don't see how. Definitely open to ideas here, as regressions will be easy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to write those tests, but pretty low confidence that that's "the right way"; pointers appreciated.
Looks okay. It shows that that shouldLintEmptyFiles
property works at least.
I tried to see if there was a way to write an integration test that the files' contents aren't read when the cache is populated, but short of modifying SourceKitten to report contents reads, or magical unix shenanigans (I think you can somehow get a pipe at a path so it gets opened like a regular file, maybe you could tell if data was consumed on the pipe?), I don't see how. Definitely open to ideas here, as regressions will be easy.
I don't know of a way to do this either.
Thanks for all your help getting this over the line, and attention to detail for the project. I really appreciate it! |
You are the one with an eye for the performance details. Keep it up! |
Per #5947, files are being read unnecessarily when they have been previously linted and appear in the cache.
To avoid this,
Linter.init
Rule.lint
to a helper, which simplifiesRule.lint
and allows it to return non-optionalRule.lint
orRule.correct
.