Baseline

[polszacki-tooploox]

Adding a baseline mechanism similar to the one implemented for Android Lint described here: https://developer.android.com/studio/write/lint#snapshot

To use the mechanism add --use-baseline flag when using the lint command. If there is no baseline file it will create .swiftlint_baseline file that's going to contain all violations reported by lint command. Calling lint with --use-baseline flag next time will only report new violations that are not present in the .swiftlint_baseline file.

[jpsim]

Thanks for the PR @polszacki-tooploox! It's a very interesting idea and one that I hadn't considered in exactly this way before. This is related to the concepts of swiftlint init that have been discussed before, where a configuration file (.swiftlint.yml) would be created to minimize the violations in a project.

I'd be happy to discuss this further after you get all the unit tests to pass again.

[jpsim]

A few thoughts:

• I'm not fully convinced this is something we should add, but I am intrigued.
• If we have a flag for this, it should probably be called --use-baseline for symmetry with our other flags and unix conventions in general
• IMO we should avoid adding a file to the root path, perhaps this is a good time to introduce a .swiftlint/ directory and the baseline file could be stored in ./swiftlint/baseline.yml or similar

[polszacki-tooploox]

@jpsim thanks for the comments!

1. The unit tests are passing again. I'll be adding some new tests for baseline functionality soon.
2. Changed the flag from --useBaseline to --use-baseline

I'll look into the .swiftlint/ directory you suggested and see what I can come up with.

[jpsim]

Looks like there's a test failure on SwiftPM with Xcode 10.0 https://dev.azure.com/jpsim/SwiftLint/_build/results?buildId=164

[Jeehut]

This idea sounds very interesting, but I have a few questions:

1. How is this supposed to be useful when using a build script? Let's say I configure it to update & use a baseline on each build, wouldn't it only show warnings & errors once and from then on ignore them?
2. How does this mode work together with // swiftlint:disable comments? Aren't they rendered basically useless? Should we ignore them when this mode is turned on? Should we even warn against them when this mode is used?
3. What if I have 10 warnings & 3 errors where 5 of the warnings should be accepted via baseline. Do I need to // swiftlint:disable the warnings and errors and then create the baseline?

I feel like this feature interferes with the disabling commands and could actually replace them. But for this to happen, we would need to be able to explicitly specify which issues should be placed into the baseline. What about this:

The --use-baseline option gets an option (e.g. disabled-only) where it will scan the code for any disable commands and will only add occurences of those issues into the baseline. When adding a disabled command to the baseline, the disable comments could be removed, so the code can stay clean. This way integration with build scripts could work quite fine as the developers decides which issues can be accepted and all others are not.

Sorry if this doesn't sound like the initial intention, but I kinda feel like this overlaps with the disable commands. Feel free to correct me and specify the use cases for this more clearly.

[mharper]

I have an iOS project that is already released at v1.0. We'd like to be able to use swiftlint going forward but would prefer to amortize our "lint debt" over time as opposed to having to fix everything Right Now™. I definitely support this feature.

[SwiftLintBot]

	12 Messages
📖	Linting Aerial with this PR took 1.82s vs 1.8s on master (1% slower)
📖	Linting Alamofire with this PR took 3.93s vs 4.21s on master (6% faster)
📖	Linting Firefox with this PR took 12.38s vs 12.71s on master (2% faster)
📖	Linting Kickstarter with this PR took 19.91s vs 19.97s on master (0% faster)
📖	Linting Moya with this PR took 1.84s vs 1.83s on master (0% slower)
📖	Linting Nimble with this PR took 1.82s vs 1.77s on master (2% slower)
📖	Linting Quick with this PR took 0.57s vs 0.55s on master (3% slower)
📖	Linting Realm with this PR took 3.34s vs 3.37s on master (0% faster)
📖	Linting SourceKitten with this PR took 1.12s vs 1.16s on master (3% faster)
📖	Linting Sourcery with this PR took 4.05s vs 4.09s on master (0% faster)
📖	Linting Swift with this PR took 26.47s vs 27.18s on master (2% faster)
📖	Linting WordPress with this PR took 21.45s vs 22.1s on master (2% faster)

Generated by 🚫 Danger

[polszacki-tooploox]

@Dschee

1. It creates the baseline only if there is no existing baseline. It should not be regenerated each time you run the lint check.
2. // swiftlint:disable are still working as they did before. Baseline check uses an output of a lint command. Issues marked with // swiftlint:disable are not reported on the output which means that those issues are not saved in a baseline.
3. It depends on what are you trying to accomplish.

The baseline mechanism is not replacing // swiftlint:disable in any way. Its purpose is to allow turning on lint checks that require refactors in existing codebase without a need to fix those first. Imagine that you have a project that was going on for 2 years. You see that the quality degrades and want to do something about it. You turn on swift lint and it hits you with 200+ errors. Without a baseline, you have to either fix all of those or forget about using swift lint. While you are fixing those 200+ errors, a new code is being merged and it adds other issues. With a baseline, you could prevent the new errors from appearing while fixing old errors at the same time.

[Jeehut]

@polszacki-tooploox Thanks for answering my questions, I'm convinced, what you describe makes sense. I think the use case should be very well documented though to prevent misunderstanding. Specifically the documentation should make sure that:

• it's clear that the command should only be run once when SwiftLint is turned on the first time
• it's clear that this doesn't have any effect on the usage of disable commands explicitly stating that warnings could still be fixed by using disable commands

Other than that, I'm happy to review this feature. Thank you for bringing it up and for the PR!

[marcelofabri]

@polszacki-tooploox I have a question about how this is used in practice: how do people usually go back and fix the warnings added to the baseline? Do they delete the file? Do they open the file to read the warnings?

[Jeehut]

@marcelofabri As I understand it, if you run the normal swiftlint lint command without the --use-baseline option, then you will see all warnings including those added to the baseline and can fix them. I'm not sure though if that's what you're asking for ...

Or are you asking about how to update the baseline when some of the issues in there are fixed?

[marcelofabri]

@marcelofabri As I understand it, if you run the normal swiftlint lint command without the --use-baseline option, then you will see all warnings including those added to the baseline and can fix them. I'm not sure though if that's what you're asking for ...

Yep, I think that answers my question :)

Or are you asking about how to update the baseline when some of the issues in there are fixed?

This is a good question, would people need to delete the file manually?

[Jeehut]

In my opinion, it would be best if any issues that are fixed are automatically removed from the baseline. That's also why I'm not a big fan of the word baseline, it implies that it's a single fixed state referred to. I'd rather rename the feature to something like --stack-of-technical-debt (though I'm not perfectly happy with that name yet) – but something that shows it's a collectino of issues to always decrease until it can be deleted. Maybe even something like --temporary-debt? I don't know ...

[Jeehut]

We just had a discussion with @fredpi about adding a .temp configuration file to our project on top of the normal configuration file when we only turn off some rule temporarily until either some false positive is fixed or for other reasons, but where we can document that it's only temporary and needs to be fixed. Then I had the idea that this baseline idea actually solves that problem, meaning that we could instead just use the --use-baseline feature. But this only stresses my point that this should be named differently, e.g. --temporary-debt.

Also it introduces the question if the baseline file should include the violations or rather the violated rules. When using this temporarily for a rule with false positives, then also new violations should be ignored. At the same time, there needs to be an option alongside the existing --strict command to ignore any violations accepted by --temporary-debt, maybe just the two combined. This implied that --temporary-debt should also work when there's no baseline file currently. Otherwise we would need to change our CI configuration all the time.

[bpollman]

Really hoping this gets finished and merged in someday. We have an existing project with thousands of swiftlint warnings that cant be easily cleaned up without significant time investment in code and testing.

Having a baseline feature (I've used several tools with a similar feature) means as the code gets refactored over time it slowly gets improved and cleaned up.

@polszacki-tooploox Do you have any plans to keep tryiong to push this through?

[fredpi]

@bpollman In #2824, I have implemented a multitude of features that all offer some kind of config merging. You could e.g. use child_config to configure a child configuration that may then temporarily disable rules.

That PR isn’t merged yet, but feature-wise, it’s complete.

[ZOlbrys]

@fredpi I'm in a similar situation as @bpollman - would you be able to give any guidance/direction (or even a sample app!?) on how we could use your PR to ignore "old" warnings/errors and only report anything new? Thank you so much!

[polszacki-tooploox]

@bpollman Yes, I still see the baseline feature as something worth doing. I used this fork in one of the biggest projects I worked on and it was really helpful and basically the only way we could start using strict SwiftLint rules without having to fix 20k+ errors in a one go.

The question is what should I do with this PR to get approvals (besides resolving conflicts).
@jpsim should I introduce .swiftlint/ directory in this PR that would contain .swiftlint.yml and a baseline file? (I like the idea but, in my opinion, it should be done in a separate PR)

@Jeehut I have to think about the naming thing. I thought that baseline was clear as it was already used in the other popular linting tool for Android development but the --temporary-debt also makes sense. What do others think?

[fredpi]

@polszacki-tooploox Maybe it would be better to create such a feature on top of what I implemented in #2824. The baseline feature would then be to automatically create a new config file with the current warnings and link it as a child_config in the .swiftlint.yml

@ZOlbrys My PR is not yet merged, but using it, you can create another configuration file and link it as a child_config. You could then use this "baseline" file to disable all rules that trigger warnings while keeping the original configuration untouched.

.swiftlint.yml

child_config: baseline.yml

opt_in_rules:
  - abc
  - dfg

baseline.yml

# Temporary Adjustments
disabled_rules:
  - dfg

[fredpi]

I just noticed that the baseline feature disables specific violations instead of violated rules, so I was talking about something different. Please ignore my other comments...

[ZOlbrys]

@polszacki-tooploox hi there! I am trying to use your baseline branch locally to test out how it might work and the impact it would have on my project. Since it is not merged to the master branch yet, I figured it would be best to build this locally using your branch and run swiftlint manually on my project.

However, when I try to use the compiled swiftlint executable, it gives me the following error:

dyld: Library not loaded: @rpath/libswiftCore.dylib
  Referenced from: /Users/zolbrys/Desktop/./swiftlint
  Reason: image not found
Abort trap: 6

(I read that this error is frequent in cases where the user is on an older version of macOS and/or doesn't have the latest Xcode versions installed, but I'm on macOS Mojave 10.14.16, and I have Xcode 11.2.1 installed.)

Building locally and running this command using the master branch works, however... any ideas?

[bpollman]

@Jeehut I have to think about the naming thing. I thought that baseline was clear as it was already used in the other popular linting tool for Android development but the --temporary-debt also makes sense. What do others think?

I have seem the term baseline used in several linters and error checking tools in C, javascript, android, etc. So it makes total sense to me to use here. That said, if there's a better term to use we should.

But I personally find --temporary-debt a bit misleading, as the code may be just fine, it just doesn't meet the listing spec. So the file being created is a list of existing warnings to be ignored for whatever reason. Its like a reference or baseline if you will :)

Regardless, I have a pretty effective swiftlint-diff script that does the job 99% of the time but it feels rather hacky and would much prefer using a native baseline feature.

Let's get this merged! (how can I help?)

[Jeehut]

Yeah, I think baseline is fine as for it's name. Let's not overthink it too much. My bad. 😅

[bpollman]

Just following up on this. Is there anything holding this back from getting merged?

[DntPullALockett]

I would love to see this added to the fastlane action as well.

use-baseline: bool

[jpsim]

I'm supportive of this kind of feature, I think it can be really useful.

I know it's hard to constantly rebase a large PR like this in the hope that a maintainer will get to review your work, and I can't commit to having time to review again, but if you do get a chance to rebase this PR I might be able to review it if I find the time.

Or maybe another maintainer will have availability to review when that's done.

[stale]

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!

[Jeehut]

Sad to see this is getting stale-botted before reviewed.

[jpsim]

Please see my latest comments here: #3421

Let's close this and hopefully continue there.