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

Resolve deprecation warnings from clap #798

Merged
merged 3 commits into from
Mar 30, 2022

Conversation

YJDoc2
Copy link
Collaborator

@YJDoc2 YJDoc2 commented Mar 27, 2022

This resolves some of the clap deprecation warnings. This is part of resolving #771 .

@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Mar 27, 2022

NOTE : The lint CI is failing due to these two only.

This does not resolve all the deprecation warnings, there are two which still remain :

warning: use of deprecated variant clap::AppSettings::AllowHyphenValues: Replaced with Command::allow_hyphen_values and Arg::is_allow_hyphen_values_set
--> liboci-cli/src/lib.rs:57:41
|
57 | #[clap(setting = clap::AppSettings::AllowHyphenValues)]
| ^^^^^^^^^^^^^^^^^
|
= note: #[warn(deprecated)] on by default

warning: use of deprecated variant clap::ArgSettings::Last: Replaced with Arg::last and Arg::is_last_set
--> liboci-cli/src/ps.rs:12:41
|
12 | #[clap(setting = clap::ArgSettings::Last)]
| ^^^^

Issue with resolving these, is that in the clap docs, both of them only have documentation for builder API, not the derive API which we use for youki. Thus in worst case, two subcommands which use these two might need to be refactored and ported to use the builder API instead of derive, and might also require some additional checks.

Copy link
Collaborator

@Furisto Furisto left a comment

Choose a reason for hiding this comment

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

Lets raise an issue with clap for the missing derive support and silence the warnings in the meanwhile.

@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Mar 28, 2022

I spent some time seeing if there was any workaround, but didn't find any. I have opened a discussion at clap : clap-rs/clap#3584

Also we don't have to silence the warnings immediately, as the CI will check them only if files in crates/* are changed. If this gets resolved before such a change occurs, then we won't need to change the CI, else we can change it temporarily in the PR where this issue occurs.

@YJDoc2 YJDoc2 force-pushed the improvements/resolve-clap-deprecations branch 2 times, most recently from f5da54f to f02005f Compare March 28, 2022 14:27
@YJDoc2 YJDoc2 force-pushed the improvements/resolve-clap-deprecations branch from f02005f to 167bf29 Compare March 29, 2022 15:00
@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Mar 29, 2022

@Furisto PTAL
This should resolve #771 completely, issue can be closed after merging this.

@YJDoc2 YJDoc2 requested a review from Furisto March 29, 2022 15:14
Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

💯

@utam0k utam0k merged commit 95d85c5 into youki-dev:main Mar 30, 2022
@YJDoc2 YJDoc2 deleted the improvements/resolve-clap-deprecations branch October 7, 2022 05:00
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.

3 participants