-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
refactor: update clap to v3 #3370
Conversation
This will likely need to be rebased on #3322 (which just got approved) |
25c2c3f
to
050b893
Compare
Removed the feature of giving arguments with environment variables, because they overwrite explicit arguments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a couple small comments 👍
src/context.rs
Outdated
|
||
/// Properties as passed on from the shell as arguments | ||
#[derive(Parser, Debug)] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Looks like a lingering newline was left here between the attribute and the definition 😄
} | ||
|
||
#[derive(Debug, Clone, Copy, PartialEq)] | ||
pub enum Target { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a doc comment here explaining what Target
is?
Explain(Properties), | ||
/// Prints the shell function used to execute starship | ||
Init { | ||
shell: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could instead create an enum with all supported shell types and derive ArgEnum
for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't do that because passing the shell path is also supported for this argument. Also, how to handle unknown shell names will require some thought. By the way, context.rs
already defines a Shell
enum.
Description
Updates clap to v3. I switched the implementation to the new
derive
/structopt
style because there were so many breaking changes.Completions are now handled by
clap-completions
.I changed some arguments to allow setting them from environment variables.Sharedmodule
/prompt
/explain
arguments are now handled bycontext::Properties
and--right
/--continuation
are handled separately forprompt
and passed on as aTarget
(the other subcommands ignore target and set it toMain
).Motivation and Context
Closes #
Screenshots (if appropriate):
How Has This Been Tested?
Checklist: