search

noeddl / ukebox

A ukulele chord toolbox in Rust
Apache License 2.0
8 stars 2 forks source link

Migrate to clap 4.x (WIP) #123

Closed schneiderfelipe closed 6 months ago

schneiderfelipe commented 7 months ago

This PR migrates to clap 4.3. This in particular requires/benefits from some changes to/on how some errors work, including depending on thiserror; I believe those are good changes overall.

I plan on doing the following in this PR:

Please let me know what you think @noeddl, in particular on the last point.

noeddl commented 7 months ago

Thanks a lot for the PR @schneiderfelipe! It looks good to me on first glance but I will take a closer look later when I have some more time, especially on the failing checks. I will probably have to update the minimum supported Rust version to allow for clap 4.

As for testing CLI validation errors: Did clap 4 improve anything in this regard or would you consider these tests superfluous no matter which clap version?

schneiderfelipe commented 7 months ago

Thanks a lot for the PR @schneiderfelipe! It looks good to me on first glance but I will take a closer look later when I have some more time, especially on the failing checks.

Some of the checks are failing due to -D warnings. In fact, some of those fail on unmodified code, maybe due to new clippy warnings having been added in the last couple years? I have some changes on my side addressing those (and other) clippy warnings, but I believe that would be outside the scope of this PR (I'm happy to contribute some of those soon 🤗)?

I will probably have to update the minimum supported Rust version to allow for clap 4.

I think the MSRV for clap 4.3 (what this PR is asking for) is 1.64.0. For comparison, clap 4.0 seems to ask for 1.60.0, so that's the interval we could choose from? cargo msrv gave me 1.64.0 if I remember correctly.

As for testing CLI validation errors: Did clap 4 improve anything in this regard or would you consider these tests superfluous no matter which clap version?

That's because some of those validations used to happen here, but since they are being proposed to happen on clap side (e.g. see here), I suggest these tests could be removed.

noeddl commented 7 months ago

I suggest that we first update the MSRV to 1.64.0 and make sure all the checks are passing (i.e. by addressing the clippy warnings) in a separate PR. Then we can rebase this PR on the new master after the other one has been merged. What do you think? If you want to you could do that using the changes that you already have in place regarding the clippy warnings.

That's because some of those validations used to happen here, but since they are being proposed to happen on clap side (e.g. see here), I suggest these tests could be removed.

I see, thanks for the clarification.

schneiderfelipe commented 7 months ago

Good call, I can do that just fine. In the meanwhile, can I ask you to rerun the CI workflow on the master branch, so that we can have a baseline?

noeddl commented 7 months ago

I had to update the CI to allow for manually triggering it in the future but now the checks have in any case run on master. 🙂 Looks like they fail at the same places as when running on this PR except for the 1.56.0 job - which is what was to be expected.

noeddl commented 6 months ago

Hi @schneiderfelipe, are you still interested in doing the preparations that we discussed above (updating the MSRV and taking care of the clippy warnings)? Otherwise I would like to do them myself so that I can merge this PR and allow for any further development.

schneiderfelipe commented 6 months ago

Hi Anett, my time got short lately. Feel free to do it if you want. I'll probably catch pace at a later time! Cheers!

Em ter, 7 de mai de 2024 06:53, Anett Seeker @.***> escreveu:

Hi @schneiderfelipe https://github.com/schneiderfelipe, are you still interested in doing the preparations that we discussed above (updating the MSRV and taking care of the clippy warnings)? Otherwise I would like to do them myself so that I can merge this PR and allow for any further development.

— Reply to this email directly, view it on GitHub https://github.com/noeddl/ukebox/pull/123#issuecomment-2097906426, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAJCBP33GTEBHIFD4LJ2GLZBCQB5AVCNFSM6AAAAABFPSZTEWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJXHEYDMNBSGY . You are receiving this because you were mentioned.Message ID: @.***>

noeddl commented 6 months ago

@schneiderfelipe Thanks again for the PR and thanks for bringing back some life to this project. 🙂