Upgrade to clap 4 #5405

dlsmith · 2023-05-13T16:46:22Z

Summary of changes

Use Arg.num_args
- Supersedes multiple_values and takes_value
Use Arg.value_parser
- Replace Arg.possible_values
- Remove use of Arg.allow_invalid_utf8, either parsing directly to PathBuf or OsString
- Move custom parsing logic from usage to arg definition (e.g., usize)
Use ArgMatches.get_one and ArgMatches.get_many in place of ArgMatches.value_of* and ArgMatches.values_of*
Use new flag/option idioms
- Arg.action(ArgAction::SetTrue) for flags
- Replace ArgMatches.is_present with ArgMatches.get_flag for flags
- Replace ArgMatches.is_present with ArgMatches.contains_id to check option presence
Remove use of App.trailing_var_arg
- This was moved to Arg and now conflicts with last. See relevant unit test.

bhansconnect

Looks great overall. Just a few minor comments.

bhansconnect · 2023-05-14T14:17:26Z

crates/cli/src/lib.rs

        .required(false);

    let flag_prebuilt = Arg::new(FLAG_PREBUILT)
        .long(FLAG_PREBUILT)
        .help("Assume the platform has been prebuilt and skip rebuilding the platform\n(This is enabled by default when using `roc build` with a --target other than `--target <current machine>`.)")
-        .possible_values(["true", "false"])
+        .value_parser(["true", "false"])


While we are here, can we flip this to use .action(ArgAction::SetTrue) instead of true and false?

Is it ok that it's not backward compatible? If so, happy to make the change.

I see the current implementation also allows you to opt out of the default behavior defined here, which we'd lose by switching to a bool.

Ultimately, this flag will go away. So exactly how it works doesn't matter too much.

I think it should be fine to just make the inside of the first branch be true and the second branch stay the same. If that makes sense.

But no big deal either way, so feel free to not fix.

bhansconnect · 2023-05-14T14:19:18Z

crates/cli/src/lib.rs

                    .help("The package's main .roc file")
-                    .allow_invalid_utf8(true)
+                    .num_args(0..0)


I... do not know how that got there. Thanks for catching it.

bhansconnect · 2023-05-14T14:20:27Z

crates/cli/src/lib.rs

                Arg::new(FLAG_TARGET)
                    .long(FLAG_TARGET)
                    .help("Choose a different target")
-                    .default_value(Target::default().into())
-                    .possible_values(Target::iter().map(|target| {
-                        Into::<&'static str>::into(target)
-                    }))
+                    .default_value(Into::<&'static str>::into(Target::default()))
+                    .value_parser(
+                        PossibleValuesParser::new(
+                            Target::iter().map(|target| {
+                                Into::<&'static str>::into(target)
+                            })
+                        ))
                    .required(false),


Given this is a bit complex, can we define it as a variable at the top and share it between all uses?

bhansconnect · 2023-05-14T23:33:17Z

Yeah, should be fine. We already talked about making this change anyway. May require changing use in a test, but should be fine to do that as well.

…

On Sun, May 14, 2023, 4:31 PM David Smith ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In crates/cli/src/lib.rs <#5405 (comment)>: > .required(false); let flag_prebuilt = Arg::new(FLAG_PREBUILT) .long(FLAG_PREBUILT) .help("Assume the platform has been prebuilt and skip rebuilding the platform\n(This is enabled by default when using `roc build` with a --target other than `--target <current machine>`.)") - .possible_values(["true", "false"]) + .value_parser(["true", "false"]) Is it ok that it's not backward compatible? If so, happy to make the change. — Reply to this email directly, view it on GitHub <#5405 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB7S22FZ2LTPOLALGQNKHWDXGFTM5ANCNFSM6AAAAAAYATAUVE> . You are receiving this because you commented.Message ID: ***@***.***>

bhansconnect · 2023-05-15T14:14:45Z

Looks great! Thanks for dealing with this update.

- Remove `num_args` call that wasn't supposed to be there - Factor out shared value parser with non-trivial init

With this change, it's no longer possible to cross-compile to any target other than wasm.

bhansconnect · 2023-05-15T16:39:47Z

I think you may need to search for prebuilt-platform= in the repo and update a few uses in tests/comments.

For comments/strings, just change uses of --prebuilt-platform=true to something like with --prebuilt-platform
and --prebuilt-platform=false to something like without --prebuilt-platform.

I think confy dependencies changes, so the sha needs to be updated Signed-off-by: Anton-4 <17049058+Anton-4@users.noreply.github.com>

bhansconnect · 2023-05-15T17:35:59Z

default.nix

@@ -23,7 +23,7 @@ rustPlatform.buildRustPackage {
  cargoLock = {
    lockFile = ./Cargo.lock;
    outputHashes = {
-      "confy-0.5.1" = "sha256-3PQdz9W/uJd4CaUZdwAd2u3JJ100SFAoKLCFE6THRZI=";
+      "confy-0.5.1" = "sha256-KML/uoze2djsFhYk488QAtauethDaC+0aZ3q56yAhuY=";


General question: how does the sha change without changing the version?

I was wondering the same thing for a while. The sha is for the built result, not for the confy source. With this PR the changing of dependencies led to some changes in dependency resolution and serde was changed to 1.0.163, serde is a dependency for confy, so confy was changed.

Anton-4 · 2023-05-15T17:49:52Z

I'll look at the clippy failure

Signed-off-by: Anton-4 <17049058+Anton-4@users.noreply.github.com>

dlsmith · 2023-05-15T20:28:12Z

Thanks for the help, @Anton-4!

Fixes omission in the [clap v4 upgrade](roc-lang#5405) See [relevant bullet](https://github.com/clap-rs/clap/blob/2d4644a8703369d3a3c50dd58c713d48ab1da2ac/CHANGELOG.md?plain=1#LL590C32-L590C67) in changelog.

dlsmith mentioned this pull request May 13, 2023

Update to clap 4 and improve subcommand typo error messages. #5388

Closed

bhansconnect reviewed May 14, 2023

View reviewed changes

dlsmith marked this pull request as ready for review May 15, 2023 00:31

bhansconnect previously approved these changes May 15, 2023

View reviewed changes

bhansconnect enabled auto-merge May 15, 2023 14:14

dlsmith added 6 commits May 15, 2023 10:40

Upgrade clap version

66f71b2

Code changes for clap 4 upgrade

0f943c3

Remove unnecessary num_args for an optional single arg

003aacb

Address review comments

54c130c

- Remove `num_args` call that wasn't supposed to be there - Factor out shared value parser with non-trivial init

Convert --prebuilt-platform to bool flag

d06d0db

With this change, it's no longer possible to cross-compile to any target other than wasm.

Updates after rebase

e1e72a3

auto-merge was automatically disabled May 15, 2023 16:32
Head branch was pushed to by a user without write access

dlsmith dismissed bhansconnect’s stale review via e1e72a3 May 15, 2023 16:33

dlsmith force-pushed the i5388 branch from a9f8cbe to e1e72a3 Compare May 15, 2023 16:33

Update --prebuilt-platform usage in comments and error messages.

1aa4524

bhansconnect previously approved these changes May 15, 2023

View reviewed changes

bhansconnect enabled auto-merge May 15, 2023 16:55

Update confy sha nix

945e0bf

I think confy dependencies changes, so the sha needs to be updated Signed-off-by: Anton-4 <17049058+Anton-4@users.noreply.github.com>

Anton-4 dismissed bhansconnect’s stale review via 945e0bf May 15, 2023 17:34

bhansconnect previously approved these changes May 15, 2023

View reviewed changes

clippy fix

dfc365b

Signed-off-by: Anton-4 <17049058+Anton-4@users.noreply.github.com>

Anton-4 dismissed bhansconnect’s stale review via dfc365b May 15, 2023 18:03

bhansconnect approved these changes May 15, 2023

View reviewed changes

bhansconnect merged commit d94b3f5 into roc-lang:main May 15, 2023

dlsmith deleted the i5388 branch May 15, 2023 20:47

Anton-4 mentioned this pull request May 17, 2023

roc help does not work anymore #5418

Closed

dlsmith mentioned this pull request May 17, 2023

Fix roc help #5419

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to clap 4 #5405

Upgrade to clap 4 #5405

dlsmith commented May 13, 2023

bhansconnect left a comment

bhansconnect May 14, 2023

dlsmith May 14, 2023

dlsmith May 15, 2023

bhansconnect May 15, 2023

bhansconnect May 14, 2023

dlsmith May 14, 2023

bhansconnect May 14, 2023

dlsmith May 14, 2023

bhansconnect commented May 14, 2023 via email

bhansconnect commented May 15, 2023

bhansconnect commented May 15, 2023 •

edited

Loading

bhansconnect May 15, 2023

Anton-4 May 15, 2023

Anton-4 commented May 15, 2023

dlsmith commented May 15, 2023

Upgrade to clap 4 #5405

Upgrade to clap 4 #5405

Conversation

dlsmith commented May 13, 2023

bhansconnect left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhansconnect commented May 14, 2023 via email

bhansconnect commented May 15, 2023

bhansconnect commented May 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anton-4 commented May 15, 2023

dlsmith commented May 15, 2023

bhansconnect commented May 15, 2023 •

edited

Loading