feat(clap_complete): Add ParseState to support more dynamic completion
#5538
Conversation
The PR / commit title say this is a feature but this looks like its a refactor (code clean up with no user-visible change). Could you clarify what the end-user impact of this either in the type or in the description? |
| @@ -53,6 +53,7 @@ pub fn complete( | |||
| let mut current_cmd = &*cmd; | |||
| let mut pos_index = 1; | |||
| let mut is_escaped = false; | |||
| let mut _state = ParseState::Unknown; | |||
There was a problem hiding this comment.
Do we need an Unknown state vs ValueDone
There was a problem hiding this comment.
I think that we need an Unknown state. ValueDone means we recognize that the word is a subcommand, flag or positional. Unknown means we don't know what is meaning of the word, for example, --flag_unknon, we parse a word starting with -- but not a arg belong to the current_cmd
There was a problem hiding this comment.
The first thing suspicious to me is that clap's parser doesn't need it, see
clap/clap_builder/src/parser/parser.rs
Lines 1605 to 1610 in 5da658c
Its also not clear what value we are supposed to be getting out of Unknown. clap's ValuesDone is more of the default state of the state machine. When you aren't currently in a specialized option or positional state, you move back to that. It sounds like Unknown is your default state, but then how is that different than your ValueDone?
Granted, it might be clearer once the state is in use. We should only include in this PR what gets used.
There was a problem hiding this comment.
Unknown and ValueDone are indeed both default states of the state machine for now, I will remove Unknown first.
| @@ -53,6 +53,7 @@ pub fn complete( | |||
| let mut current_cmd = &*cmd; | |||
| let mut pos_index = 1; | |||
| let mut is_escaped = false; | |||
| let mut _state = ParseState::Unknown; | |||
There was a problem hiding this comment.
_state is still an unused variable. Starting with _ is just to prevent warnings.
There was a problem hiding this comment.
When presented with a warning like that, it would be good to consider carefully before "suppressing" it. In this case, the compiler was basically telling you that the commit is not atomic.
| } else if let Some((flag, value)) = arg.to_long() { | ||
| if let Ok(flag) = flag { | ||
| _state = if let Some(opt) = current_cmd.get_arguments().find(|a| { | ||
| a.get_long_and_visible_aliases() | ||
| .map_or(false, |v| v.into_iter().find(|s| *s == flag).is_some()) | ||
| }) { | ||
| match opt.get_action() { | ||
| clap::ArgAction::Set | clap::ArgAction::Append => { | ||
| if value.is_some() { | ||
| ParseState::ValueDone | ||
| } else { | ||
| ParseState::Opt(opt.clone()) | ||
| } | ||
| } | ||
| _ => { | ||
| if value.is_some() { | ||
| ParseState::Unknown | ||
| } else { | ||
| ParseState::ValueDone | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| ParseState::Unknown | ||
| }; | ||
| } | ||
| } else if let Some(short) = arg.to_short() { | ||
| let mut short = short.clone(); | ||
| let opt = short.next_flag(); | ||
|
|
There was a problem hiding this comment.
I thought this PR was supposed to just be a refactor that added ParseState::ValueDone and ParseState::Pos. I believe the intention was to remove the use of passing is_escaped to complete_arg.
There was a problem hiding this comment.
This PR is supposed to add a struct to record the context during parsing. Actually, ParseState::Opt is the key filed in this PR. We could use it to complete --flag bar and -f bar. And i think that ParseState could be expanded if there are other contexts.
I believe the intention was to remove the use of passing is_escaped to complete_arg.
Yes, we can do this. is_escaped is like a context variable for the word after --
There was a problem hiding this comment.
The PR / commit title say this is a feature but this looks like its a refactor (code clean up with no user-visible change). Could you clarify what the end-user impact of this either in the type or in the description?
This PR does not provide new features for users, but it provides a feature for developers to record more context. From the user's perspective, this PR should perhaps be labeled as a refactor.
There was a problem hiding this comment.
This PR is supposed to add a struct to record the context during parsing.
That is not the reason I suggested making this PR and this not a mergeable direction for this PR. Commits need to be atomic which this is not. PRs can either be a single commit or multiple that "tell a story together".
a feature for developers to record more context.
That is a refactor, not a feature. The terms are user-facing.
There was a problem hiding this comment.
I don't think this counts as a refactor either. it's the codebase for the new feature #5539. The reason I separated it is that I think this PR could may also serve as the code base for other features, if someone wants to support multivalue completion, he could add a field in ParseState and add some logic in complete function.
From the user's perspective, this PR is indeed not atomic. So perhaps I should merge the code from PR #5539 into this PR and then close it? I'll keep the two commits to make it easier for everyone to track.
Related issue: #3920
Add
ParseStateto record the context during completion parsing. So when we do dynamic completion, we can use this context to specify what we need to complete and support more dynamic completion including--flag value