Add --lint-level cli option

[Finomnis]

Closes #359

[obi1kenobi]

Seems reasonable to me. Deferring to @epage on the final decision since this is somewhat relevant to the merge into cargo, and he hasn't had a chance to chime in on this yet.

In a chat with @tonowak, we realized that an integration test over any one of the test crates for testing #[must_use] (e.g. this one) can be used to test this feature: running with --lint-level major should not report any lints across the old/new version of that crate.

If you'd like to add that test, great! If not, I can add it post-merge.

[Finomnis]

@obi1kenobi I just tried and failed ... If you could add it, that would be great. Feel free to push directly to this PR.

[epage]

Don't these two checks to the same thing, just from different sources? I feel like the lint_level flag should be Option<LintLevel> and if its None, then in the above code we use the version_change logic to set it. We then only need one filter on the query to perform.

[Finomnis]

I thought this was the whole point. We don't want to override it, only filter it. If the version changed by a major increment, we don't want to run minor checks no matter what the lint level is set to.

The goal of this flag is to hide checks that are irrelevant to the user, just like the other filter hides checks that are irrelevant based on the existing version difference in Cargo.toml. Those two are orthogonal, one does not imply the other. Both have to be valid for the lint to run.

[Finomnis]

You are proposing pretty much exactly the change I had earlier (--assume-semver) which was rejected for that reason.

[epage]

This is being further discussed in the issue

[epage]

FYI you can use ValueEnum to implement this: https://github.com/clap-rs/clap/blob/master/src/util/color.rs#L67

[epage]

imo we should have a Patch level, even if we don't use it yet but I leave that to you all

[Finomnis]

That doesn't make sense, what would a Patch level lint be? A Minor lit checks if the API changes require a minor version increment. There is nothing less than a Patch level increment; so checking for things that would require a Patch level increment makes no sense. That's why RequiredSemverUpdate only has those two options as well.

I think there still is a misunderstanding / misconsensus what we are trying to achieve here.

The essential question for me is: do you agree with this statement?

I now implemented it so that two things have to be given for a lint to run:

• The version of the Cargo.toml and the currently published version must make the lint relevant; if the version in Cargo.toml is a major version change, nothing should run because everything is allowed
• The --lint-level flag must not hide the lint; if --lint-level is major, checks for minor lints are hidden

Also note the help message of the --lint-level flag:

/// Skip lints that fall below the given semver level

This is not a "ignore version difference and instead assume the given lint level" flag.
It is a "from the lints that would be relevant for the given version change, only run the ones that are at least [lint-level]".

The background is that by default, if two versions are identical, the code behaves as if a patch level increment was made. In tokio, however, we always make minor increments, and want to completely ignore patch increments.

So --lint-level major would silence all minor checks, because we by default assume that the next version will be a minor increment anyway.

That said, we don't want to fully override the version difference to minor always, because we might at some point release a new major version. So simply filtering out the minor lints is exactly what would suit our needs.

[Finomnis]

@epage As I said earlier, there are two things that seem to cause confusion in the discussions:

• Overriding the version difference vs additional filtering after the version check
• The confusing correspondence between filter levels, lint levels and version increment levels.
  • Minor lints check if a minor version increment is required. Those checks should be silenced if a minor version increment was already detected
    • --lint-level major means that only major tests should run, meaning we only check whether or not we need a major version increment. Therefore this flag the same effect as a minor version increment (this is the part that confuses people, I think; but without looking at the implementation details, it's the most intuitive way)
  • Major lints check if a major version increment is required; those checks should be silenced if a major version increment was detected

Maybe this is more intuitive:

• --lint-level minor checks for both major and minor lints and fails if the new version in Cargo.toml does not match those changes
• --lint-level major only checks for major lints and ignores minor lints. It also only fails if the version in Cargo.toml does not match those changes.

Or maybe even shorter:

• --lint-level minor behaves as the tool did before
• --lint-level major skips minor lints even if they would be required

[Finomnis]

Alternatively, we could introduce a flag --skip-minor-lints that would do the same thing as --lint-level major. Maybe that would be more intuitive.

[Finomnis]

Rejected in favor of #361