-
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
base: main
Are you sure you want to change the base?
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.
@@ -378,7 +381,13 @@ public struct CollectedLinter { | |||
} | |||
|
|||
var corrections = [Correction]() | |||
for rule in rules.compactMap({ $0 as? any CorrectableRule }) { | |||
for rule in rules.compactMap({ (rule: any Rule) -> (any CorrectableRule)? in |
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.
Stylistically, I would prefer a separate if
statement in the for
block to check for shouldRun
. Or you can make use of a where
clause.
We could even check for CorrectableRule
in the for
block to avoid creating a new list and iterating twice.
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. |
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
.