Add "stable git revision" CLI option #3536
Conversation
Generated by 🚫 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 ( |
|
There are a few correctness issues here:
There might be a way to integrate into the existing A naive way to solve the first issue could be to check if there are any files named 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 |
AvdLee
left a comment
There was a problem hiding this comment.
I really like this addition, it will definitely help us for WeTransfer/WeTransfer-iOS-CI#67.
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.
E.g. sometimes your local changes will be merged into develop while another time you might want to merge into another branch. I guess in our case, defaulting to HEAD would make the most sense 🤔
| If specified, SwiftLint 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. | ||
| [jpsim](https://github.com/jpsim) |
There was a problem hiding this comment.
Would it be valuable to default to HEAD?
There was a problem hiding this comment.
I’d rather not have a default at all because of the aforementioned correctness pitfalls.
There was a problem hiding this comment.
Makes sense! It's fairly simple as well to provide a default on the implementation level 👌
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". |
@jpsim I didn't think about this thoroughly, but want to mention that for remote configurations, the folder 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... |
I think I struck a good balance between correctness and usefulness with my latest change. If any files named 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 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! |
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.
Between HEAD and the stable git revision. Also include untracked files.
a872373 to
adc92db
Compare
|
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 |
|
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? |
|
Please merge this! |
|
@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. |
|
I would definitely use this |
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.