SwiftLint icon indicating copy to clipboard operation
SwiftLint copied to clipboard

Implement baseline functionality

Open PaulTaykalo opened this issue 4 years ago • 12 comments

This is the rebased version of #2580

PaulTaykalo avatar Nov 09 '20 20:11 PaulTaykalo

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

SwiftLintBot avatar Nov 09 '20 20:11 SwiftLintBot

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!

stale[bot] avatar Jan 09 '21 03:01 stale[bot]

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.

dhoskins avatar Jan 13 '21 17:01 dhoskins

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.

bpollman avatar Feb 22 '21 00:02 bpollman

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.

  1. Should users be able to specify the baseline file path?
  2. Should baselines apply to analyzer rules, or just lint rules?
  3. 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?
  4. Should there be an easy way to reset the baseline? Right now you have to find the baseline file and delete it.
  5. 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?
  6. Unit tests, documentation and changelog entries would be important to add before considering merging something like this.

jpsim avatar Feb 23 '21 17:02 jpsim

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

jpsim avatar Feb 23 '21 17:02 jpsim

+1 for that feature

igormatos avatar Jun 08 '21 20:06 igormatos

+1 for that feature

iandreyshev avatar Jul 08 '22 12:07 iandreyshev

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

abhimaandunzo avatar Dec 01 '22 17:12 abhimaandunzo

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 ?

AlfredAfutu avatar Apr 11 '23 08:04 AlfredAfutu

Please a lot of people are waiting for that.

aiKrice avatar Dec 06 '23 07:12 aiKrice

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.

  1. 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.

  1. Should baselines apply to analyzer rules, or just lint rules?

I think so, but I would implement this (in CI) with separate baseline files.

  1. 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.

  1. 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.

  1. 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.

  1. Unit tests, documentation and changelog entries would be important to add before considering merging something like this.

mildm8nnered avatar Jan 14 '24 23:01 mildm8nnered

very eager to have this feature !!!

aiKrice avatar Feb 24 '24 19:02 aiKrice

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.

mildm8nnered avatar Feb 25 '24 22:02 mildm8nnered