SwiftLint
SwiftLint copied to clipboard
Implement baseline functionality
This is the rebased version of #2580
2 Warnings | |
---|---|
:warning: | Please include a CHANGELOG entry to credit yourself! You can find it at CHANGELOG.md. |
:warning: | This PR may need tests. |
12 Messages | |
---|---|
:book: | Linting Aerial with this PR took 1.96s vs 2.02s on master (2% faster) |
:book: | Linting Alamofire with this PR took 2.78s vs 2.81s on master (1% faster) |
:book: | Linting Firefox with this PR took 9.36s vs 9.46s on master (1% faster) |
:book: | Linting Kickstarter with this PR took 15.58s vs 15.63s on master (0% faster) |
:book: | Linting Moya with this PR took 1.44s vs 1.48s on master (2% faster) |
:book: | Linting Nimble with this PR took 1.45s vs 1.38s on master (5% slower) |
:book: | Linting Quick with this PR took 0.63s vs 0.63s on master (0% slower) |
:book: | Linting Realm with this PR took 4.51s vs 4.6s on master (1% faster) |
:book: | Linting SourceKitten with this PR took 1.11s vs 1.06s on master (4% slower) |
:book: | Linting Sourcery with this PR took 7.24s vs 7.2s on master (0% slower) |
:book: | Linting Swift with this PR took 11.12s vs 11.2s on master (0% faster) |
:book: | Linting WordPress with this PR took 17.63s vs 17.88s on master (1% faster) |
Here's an example of your CHANGELOG entry:
* Implement baseline functionality.
[PaulTaykalo](https://github.com/PaulTaykalo)
[#issue_number](https://github.com/realm/SwiftLint/issues/issue_number)
note: There are two invisible spaces after the entry's text.
Generated by :no_entry_sign: Danger
This issue has been automatically marked as stale because it has not had any recent activity. Please comment to prevent this issue from being closed. Thank you for your contributions!
This issue has been automatically marked as stale because it has not had any recent activity. Please comment to prevent this issue from being closed. Thank you for your contributions!
I'd like to prevent this issue from being closed.
I have been hoping for this branch/feature to make it into the main release for a very long time. I'd be happy to help. What are the chances for this to get reviewed and have a chance at getting merged.
Looking at this again for the first time in a while, there's a few things I'd like the author(s) to think through before we can consider something like this, which I think overall the concept is a good idea.
- Should users be able to specify the baseline file path?
- Should baselines apply to analyzer rules, or just lint rules?
- Right now the baseline implementation is fairly brittle in the sense that if the line numbers change, the baseline record becomes out of date. Do we want to consider a mechanism to minimize the thrash when say a violation is moved down by a few lines because new code was added above it?
- Should there be an easy way to reset the baseline? Right now you have to find the baseline file and delete it.
- Can we somehow leverage the lint results cache? We're already saving lint results for faster incremental linting operations, so perhaps snapshotting that would be a new command distinct from
swiftlint lint
? - Unit tests, documentation and changelog entries would be important to add before considering merging something like this.
As I was reviewing this, I was making some edits, which I always find helpful in understand code, they may or may not be helpful to anyone who's interested in continuing this work: https://github.com/realm/SwiftLint/commits/jp-baseline
+1 for that feature
+1 for that feature
do we have this baseline implementation in pipeline . I dont see the any urgency in merging this pr. Let us know what is stopping may be we can help
This is obviously needed as mentioned by a good number of people including myself. Is there a plan on how to proceed with this @jpsim ?
Please a lot of people are waiting for that.
I'm interested in picking this up. Not sure when I'll have time, or if I'll start from here, or from scratch right now.
Looking at this again for the first time in a while, there's a few things I'd like the author(s) to think through before we can consider something like this, which I think overall the concept is a good idea.
- Should users be able to specify the baseline file path?
I think so. My use case is that I will almost always want the baseline to be derived from my main branch.
So I think something like --write-baseline <filename>
and --read-baseline <filename>
perhaps.
- Should baselines apply to analyzer rules, or just lint rules?
I think so, but I would implement this (in CI) with separate baseline files.
- Right now the baseline implementation is fairly brittle in the sense that if the line numbers change, the baseline record becomes out of date. Do we want to consider a mechanism to minimize the thrash when say a violation is moved down by a few lines because new code was added above it?
So I was thinking something like, if the file has the same violations, in the same order, ignore the violations regardless of line number.
Not sure how to handle existing violations if say one new violation appearing in a file - whether to report all the violations in the file, or to try to detect which one is "new".
For example, if my baseline violations are
identifier_name
, line 20
and my new violations are
identifier name
, line 25
identifier name
, line 30
which is the new one
Possibly ignore them if the line numbers match exactly, but otherwise report them all.
- Should there be an easy way to reset the baseline? Right now you have to find the baseline file and delete it.
I think if we supported --write-baseline <filename>
and --read-baseline <filename>
with the same <filename>
, that would work.
- Can we somehow leverage the lint results cache? We're already saving lint results for faster incremental linting operations, so perhaps snapshotting that would be a new command distinct from
swiftlint lint
?
I would definitely take a look at that.
- Unit tests, documentation and changelog entries would be important to add before considering merging something like this.
very eager to have this feature !!!
So I have a very basic implementation at #5475
Seems to work for my basic test case, but will try to work on it and to add some more docs next week.