SwiftLint
SwiftLint copied to clipboard
realm
Add "stable git revision" CLI option
This allows specifying a git revision or "commit-ish" that is considered stable. If specified, SwifLint will attempt to query the git repository index for files changed since that revision, using only those files as input as opposed to traversing the file system to collect lintable files.
| 12 Messages | |
|---|---|
| :book: | Linting Aerial with this PR took 2.5s vs 2.49s on master (0% slower) |
| :book: | Linting Alamofire with this PR took 3.72s vs 3.76s on master (1% faster) |
| :book: | Linting Firefox with this PR took 12.49s vs 12.46s on master (0% slower) |
| :book: | Linting Kickstarter with this PR took 18.99s vs 18.91s on master (0% slower) |
| :book: | Linting Moya with this PR took 1.67s vs 1.65s on master (1% slower) |
| :book: | Linting Nimble with this PR took 1.51s vs 1.51s on master (0% slower) |
| :book: | Linting Quick with this PR took 0.65s vs 0.65s on master (0% slower) |
| :book: | Linting Realm with this PR took 4.9s vs 4.88s on master (0% slower) |
| :book: | Linting SourceKitten with this PR took 1.23s vs 1.22s on master (0% slower) |
| :book: | Linting Sourcery with this PR took 10.01s vs 10.03s on master (0% faster) |
| :book: | Linting Swift with this PR took 14.38s vs 14.41s on master (0% faster) |
| :book: | Linting WordPress with this PR took 23.54s vs 23.64s on master (0% faster) |
Generated by :no_entry_sign: Danger
Good discussion offline with Keith, I think I'll remove this from the configuration and make it a CLI argument. This will make it easier to run against all files say on CI and use this faster mode locally without having to edit the config file. SwiftLint should also be plenty fasts for small projects even when linting everything, so if you can handle the complexity of a large Swift project, you can handle the complexity of running SwiftLint with a flag (swiftlint --stable-git-revision=origin/main).
There are a few correctness issues here:
- If any of the configuration files used by SwiftLint are modified, that should force all files to be re-linted, but currently this is ignored
- Same thing when the Swift version changes.
There might be a way to integrate into the existing LinterCache caching system to resolve this, but it's not clear to me that there's a way to do that without first traversing the file system, which in many ways defeats the purpose of this feature.
A naive way to solve the first issue could be to check if there are any files named .swiftlint.yml that were modified and then fall back to using the standard FileManager.default lintable file manager, but this doesn't handle the case where configuration files with different names are used, or if a configuration file that's not tracked in git is specified.
Maybe this feature is still valuable enough with these correctness to ship as-is, but I'd like to think about our options a bit more.
I'd be curious to hear some thoughts from contributors on this, especially the limitations involved and if anyone has ideas for resolving them. @keith @marcelofabri @fredpi @otaviocc
The thing I'm asking myself is how I would be able to dynamically specify the revision to compare to. I also realise our current implementation is far from ideal in this sense.
I think it’s best to see this option as an optimization only. If you have a branch that you keep free of lint violations, then you can specify it as the stable git branch. Or if you run SwiftLint as a pre-commit check, specifying HEAD should work in that case. It’s “ok” if you lint more files than have actually changed, those will be skipped if they’re in the SwiftLint cache.
I think it’s best to see this option as an optimization only. If you have a branch that you keep free of lint violations, then you can specify it as the stable git branch. Or if you run SwiftLint as a pre-commit check, specifying HEAD should work in that case. It’s “ok” if you lint more files than have actually changed, those will be skipped if they’re in the SwiftLint cache.
Thanks for that explanation, it makes things more clear. I guess that makes sense and would even be a great fit for our custom implementation too, as we indeed have such "lint-violation-free branch".
There are a few correctness issues here:
- If any of the configuration files used by SwiftLint are modified, that should force all files to be re-linted, but currently this is ignored
- Same thing when the Swift version changes.
There might be a way to integrate into the existing
LinterCachecaching system to resolve this, but it's not clear to me that there's a way to do that without first traversing the file system, which in many ways defeats the purpose of this feature.A naive way to solve the first issue could be to check if there are any files named
.swiftlint.ymlthat were modified and then fall back to using the standardFileManager.defaultlintable file manager, but this doesn't handle the case where configuration files with different names are used, or if a configuration file that's not tracked in git is specified.Maybe this feature is still valuable enough with these correctness to ship as-is, but I'd like to think about our options a bit more.
@jpsim I didn't think about this thoroughly, but want to mention that for remote configurations, the folder .swiftlint/RemoteConfigCache is created (and .swiftlint gets automatically added to the .gitignore). Maybe we could change it so that the that the .swiftlint/RemoteConfigCache gets added to the .gitignore while the .swiftlint folder itself doesn't get ignored. We could then use that folder to cache hashes for the configuration used for each individual file. If the hash changes for one file, that means that some configuration (maybe also a nested configuration) has changed which could then trigger a relint of all files. The big (and probably too big) disadvantage of such a behavior would be that SwiftLint generates files that get committed to git...
But it's the only solution I can think of which actually works – there's no other way to e. g. catch the change of a remote configuration...
The best option though would probably be to just don't add configuration change management...
- If any of the configuration files used by SwiftLint are modified, that should force all files to be re-linted, but currently this is ignored
- Same thing when the Swift version changes.
I think I struck a good balance between correctness and usefulness with my latest change.
If any files named .swiftlint.yml, or the configuration file(s) passed using --config have changed according to git, we'll fall back to linting all files as found by traversing the file system.
This won't work if remote configurations change, or if the root configuration is not tracked by git, whether because it's out of tree or in .gitignore.
I don't think it's very useful to fall back to linting all files when the Swift version changes because we don't store the Swift version used when the stable git revision was considered stable.
All in all, I think this is ready for usage as-is. My recommendation would be that this be used only as a local development optimization "fast-path" and that a continuous integration system always perform a full lint (although that full lint can also be accelerated by the linter cache, which isn't vulnerable to any known correctness issues).
I'll still need to write some documentation around this before we can merge though.
If anyone has objections or suggestions, please let me know!
Chiming in to say I'd definitely use this and it looks like it's in limbo. We'd like to run SwiftLint only on files changed vs our main branch as an Xcode build step, and linting the entire codebase takes too long.
@stevelandeyasana Can you try this out this branch and let me know if this works for your workflows? You might want to pull in recent changes from the master branch too.
Hello everyone, this very useful and time saving addition is laying around almost a year now and still hasn't found it's way into a release. Is this even considered to get released?
@ObjectiveCesar @getogrand have you used this in your workflows? Have you hit any issues, does this behave how you'd expect? I'm looking for feedback before merging.
@ObjectiveCesar @getogrand have you used this in your workflows? Have you hit any issues, does this behave how you'd expect? I'm looking for feedback before merging.
Not yet, but I'm going to build this and try it out on our commercial source code, which has 63k Swift cloc. It may take a few weeks.
