Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate from structopt to clap v3 #1293

Merged
merged 3 commits into from Apr 11, 2022

Conversation

david-perez
Copy link
Contributor

clap v3 added support for a structopt-like parser derive usage.
structopt is now in maintenance mode
(https://docs.rs/structopt/latest/structopt/#maintenance); it is
recommended to migrate to clap.

Structs to parse CLI arguments have been renamed to Args for
consistency.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

`clap` v3 added support for a `structopt`-like parser derive usage.
`structopt` is now in maintenance mode
(https://docs.rs/structopt/latest/structopt/#maintenance); it is
recommended to migrate to `clap`.

Structs to parse CLI arguments have been renamed to `Args` for
consistency.
@david-perez david-perez requested a review from a team as a code owner April 1, 2022 11:19
@github-actions
Copy link

github-actions bot commented Apr 1, 2022

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Apr 1, 2022

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Server Test

@github-actions
Copy link

github-actions bot commented Apr 1, 2022

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement Deviation Current Old
Requests/sec -5.03% 76110.22 80138.18
Total requests -5.06% 6854497 7219759
Total errors NaN% 0 0
Total successes -5.06% 6854497 7219759
Average latency ms -24.14% 0.88 1.16
Minimum latency ms 0.00% 0.02 0.02
Maximum latency ms -23.02% 20.9 27.15
Stdev latency ms -28.78% 1.46 2.05
Transfer Mb -5.06% 712.53 750.5
Connect errors NaN% 0 0
Read errors NaN% 0 0
Write errors NaN% 0 0
Status errors (not 2xx/3xx) NaN% 0 0
Timeout errors NaN% 0 0

Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to do this!

/// The path to the smithy-rs repo folder.
#[structopt(long, parse(from_os_str))]
#[clap(long, parse(from_os_str))]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% on this, but the parse(from_os_str) might not be needed on the PathBufs anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICS they're still needed to parse non-UTF-8 paths clap-rs/clap#3496.

@@ -50,7 +50,7 @@ impl Opt {
}

fn main() -> anyhow::Result<()> {
let opt = Opt::from_args().validate()?;
let opt = Args::parse().validate()?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like CI is failing since the Docker image tests that all the tools are in the right place by calling them with their --version flag, and I guess structopt must have been giving us that flag for free.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Fixed in 5365a83

@github-actions
Copy link

github-actions bot commented Apr 7, 2022

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Apr 7, 2022

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Server Test

@github-actions
Copy link

github-actions bot commented Apr 7, 2022

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement Deviation Current Old
Requests/sec -7.72% 35573.65 38550.81
Total requests -7.75% 3203472 3472631
Total errors NaN% 0 0
Total successes -7.75% 3203472 3472631
Average latency ms 5.43% 0.97 0.92
Minimum latency ms 50.00% 0.03 0.02
Maximum latency ms -10.29% 18.4 20.51
Stdev latency ms -9.09% 0.8 0.88
Transfer Mb -7.75% 333 360.98
Connect errors NaN% 0 0
Read errors NaN% 0 0
Write errors NaN% 0 0
Status errors (not 2xx/3xx) NaN% 0 0
Timeout errors NaN% 0 0

@david-perez david-perez force-pushed the davidpz-migrate-from-`structopt`-to-`clap`-v3 branch from c9e8835 to 959fc45 Compare April 11, 2022 18:21
@github-actions
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Server Test

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement Deviation Current Old
Requests/sec 10.80% 41613.7 37559.12
Total requests 10.84% 3748811 3382178
Total errors NaN% 0 0
Total successes 10.84% 3748811 3382178
Average latency ms -6.74% 0.83 0.89
Minimum latency ms -33.33% 0.02 0.03
Maximum latency ms -18.30% 16.56 20.27
Stdev latency ms 6.15% 0.69 0.65
Transfer Mb 10.84% 389.69 351.58
Connect errors NaN% 0 0
Read errors NaN% 0 0
Write errors NaN% 0 0
Status errors (not 2xx/3xx) NaN% 0 0
Timeout errors NaN% 0 0

@jdisanti jdisanti merged commit 3c25664 into main Apr 11, 2022
@jdisanti jdisanti deleted the davidpz-migrate-from-`structopt`-to-`clap`-v3 branch April 11, 2022 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants