Migrate clap to version 4 with derives

[MiniaczQ]

Motivation:

Current app uses an old version of clap crate with the builder convention, which adds a lot of additional runtime uncertainty and makes it difficult to separate the arguments into multiple apps/subcommands. Using the currently available derive convention will make it easier to group arguments together and make use of compile time checks.

Changes:

• Update the clap crate to v4
• Migrate arguments to structs with derives

[MiniaczQ]

I think this is the least changes I could possibly do.
On the way I ran into some questionable code, but I decided to keep it as to not make it harder to review this already monstrous PR.

[MiniaczQ]

One test does not pass after the refactor: exclude_paths_directory_separators.
It uses what I think is an invalid convention:

let args = TarpaulinCli::parse_from(vec![
            "tarpaulin",
            "--exclude-files",
            "src/foo/*",
            // No `--exclude-files` before the next argument
            "src\\bar\\*",
        ]);

[MiniaczQ]

I wanna just ignore it and add the missing --exclude-files, what do you think?

[xd009642]

in clap 3 I think multiple_values=true worked for getting the same behaviour - maybe it exists in clap 4? Just on my way out so I'll try and think about it more and reply tomorrow or monday but I remember personally finding doing --exclude-files multiple times a bit annoying but if it's a convention I suppose I could shift to it

[MiniaczQ]

I'll check again, it could be one of those builder-only things

[MiniaczQ]

Yeah, so it exists in clap 3 and disappears in clap 4, either they renamed it or it's totally gone

[MiniaczQ]

I found it, extremely obscure and undocumented, even in the source code I couldn't find it:
clap-rs/clap#4998 (comment)

I'll patch it in a second, just wanna write an issue on clap for this

[MiniaczQ]

The next step would be to separate arguments for building tests, running tests and processing results.
I think we can also perform this separation on Config class with the #[serde(flatten)] without breaking compatibility yet.

[MiniaczQ]

Oh yeah, about the convention of multiple values per flag,
https://docs.rs/clap/latest/clap/struct.Arg.html#method.allow_hyphen_values
describes how this option can lead to problems, I guess that's why it's rarely done that way

[MiniaczQ]

Should I work on the next changes in this PR or a new one?

[xd009642]

A new one please, I'm trying to find some time to review this one as it is (part way through it). And when that's done I'll merge it in. More changes come in it'll slow down the review a bit 😅

[xd009642]

there are some conflicts to resolve though - before merge

[xd009642]

Just a few comments both of which are code formatting. I'll also clone your fork and do a quick sanity check to make sure things look how I expect from a UX perspective

[xd009642]

style nit, I'd prefer the empty lines between fields to be removed. But if this is my only comment on this PR can ignore it

[MiniaczQ]

In the long run I'd lean towards this too, it was easier to work with in the 200-line struct but the goal is to make it smaller anyways

[xd009642]

Did cargo fmt really do this?

[MiniaczQ]

Yup

[xd009642]

Okay one regression cargo tarpaulin --help no longer works, it only works with cargo-tarpaulin --help . I think that's the only actual issue I'd need addressing before this can be merged

[xd009642]

Getting closer, it actually shows some printout from tarpaulin not cargo

cargo tarpaulin --help
error: unexpected argument 'tarpaulin' found

Usage: cargo-tarpaulin [OPTIONS] [-- <ARGS>...]

For more information, try '--help'.

[MiniaczQ]

I tried a few thing but nothing worked like it should so I posted a question on clap's discussion forum
clap-rs/clap#5107

hopefully I missed something

[MiniaczQ]

Very quick response from the clap team, there are 2 working solutions, I went with the less bloaty one, felt more in line with what I originally tried.

[xd009642]

LGTM thanks for the hard work! Once CI passes I'll merge this in 🚀