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

Support allow_hyphen_values in native completions #5628

Merged
merged 4 commits into from
Sep 5, 2024

Conversation

mart-mihkel
Copy link
Contributor

Resolves #5594

@epage
Copy link
Member

epage commented Aug 6, 2024

@shannmu would be interested in you looking over this as well, both from your experience with this code and for where you were planning on taking this code.

@shannmu
Copy link
Contributor

shannmu commented Aug 10, 2024

Sorry for the late reply, I will look over the code.

@shannmu
Copy link
Contributor

shannmu commented Aug 10, 2024

Thank you very much for your attention to this issue!

if is_escaped {
(state, pos_index) = parse_positional(current_cmd, pos_index, is_escaped, state);
} else if arg.is_escape() {
is_escaped = true;
state = ParseState::ValueDone;
} else if let Some((flag, value)) = arg.to_long() {
if let Ok(flag) = flag {
let opt = current_cmd.get_arguments().find(|a| {
let longs = a.get_long_and_visible_aliases();
let is_find = longs.map(|v| {
let mut iter = v.into_iter();
let s = iter.find(|s| *s == flag);
s.is_some()
});
is_find.unwrap_or(false)
});
state = match opt.map(|o| o.get_action()) {
Some(clap::ArgAction::Set) | Some(clap::ArgAction::Append) => {
if value.is_some() {
ParseState::ValueDone
} else {
ParseState::Opt((opt.unwrap(), 1))
}
}
Some(clap::ArgAction::SetTrue) | Some(clap::ArgAction::SetFalse) => {
ParseState::ValueDone
}
Some(clap::ArgAction::Count) => ParseState::ValueDone,
Some(clap::ArgAction::Version) => ParseState::ValueDone,
Some(clap::ArgAction::Help)
| Some(clap::ArgAction::HelpLong)
| Some(clap::ArgAction::HelpShort) => ParseState::ValueDone,
Some(_) => ParseState::ValueDone,
None => ParseState::ValueDone,
};
} else {
state = ParseState::ValueDone;
}
} else if let Some(short) = arg.to_short() {
let (_, takes_value_opt, mut short) = parse_shortflags(current_cmd, short);
match takes_value_opt {
Some(opt) => {
state = match short.next_value_os() {
Some(_) => ParseState::ValueDone,
None => ParseState::Opt((opt, 1)),
};
}
None => {
state = ParseState::ValueDone;
}
}
} else {
match state {
ParseState::ValueDone | ParseState::Pos(..) => {
(state, pos_index) =
parse_positional(current_cmd, pos_index, is_escaped, state);
}
ParseState::Opt((opt, count)) => match opt.get_num_args() {
Some(range) => {
let max = range.max_values();
if count < max {
state = ParseState::Opt((opt, count + 1));
} else {
state = ParseState::ValueDone;
}
}
None => {
state = ParseState::ValueDone;
}
},
}
}
}

This code is primarily used to match patterns in the arg string itself, such as --, --flag, -f, and value. These different patterns go into different branches. Adding an if allow_hyphen branch in there, I believe, would disrupt the logic of this code.

I think we could try matching the state in other branches, similar to how it's done in the else branch.
Here’s an example:

else if let Some((flag, value)) = arg.to_long() {
    if let ParseState::Opt(opt, count) = state {
        if opt.is_allow_hyphen_values_set() {
            // update state
        }
    }
}

Of course, the logic for allow_hyphen_values should be similar across these branches, so perhaps the code can be reused.

@shannmu
Copy link
Contributor

shannmu commented Aug 10, 2024

Another case we need to test for and handle is when we are in the ValueDone state, if the long or short is unknown and the next positional allows hyphen values, then we will take the unknown argument and treat it as a value.

As mentioned above, we need to add some logic in each branch. If the result of current_cmd.get_arguments().find() is None and the next positional argument allows hyphen values, we need to update pos_index.
Additionally, we also need to make some adjustments in complete_arg to support the complete positional arguments that allow hyphen values.

No need to worry about the code getting bloated; if it does, that just means it’s time for a refactor.

@mart-mihkel mart-mihkel marked this pull request as draft August 15, 2024 05:57
@epage
Copy link
Member

epage commented Aug 15, 2024

I've been trying to do some clean up to make it easier for people to be contributing to this code. Things should be calmed down for resolving conflicts.

@mart-mihkel
Copy link
Contributor Author

I'm a bit unsure what I should change in the arg.is_escape case and in complete_arg

@shannmu
Copy link
Contributor

shannmu commented Aug 19, 2024

current_cmd.get_arguments().find() is None means the current raw_arg looks like an option but it's not. If the current positional argument allows hyphen values, we could treat it as a positional argument, so we need to update pos_index.

Additionally, we also need to make some adjustments in complete_arg to support the complete positional arguments that allow hyphen values.

I seem to have misunderstood; updating pos_index is enough. There are no restrictions on completing positional arguments in complete_arg.

@mart-mihkel mart-mihkel marked this pull request as ready for review August 21, 2024 06:10
@epage
Copy link
Member

epage commented Aug 22, 2024

Could you clean up your commits so they are presented for easy understanding of what you are trying to accomplish and without any merge commits?

@epage
Copy link
Member

epage commented Sep 5, 2024

Thanks!

@epage epage merged commit 6b7aa3d into clap-rs:master Sep 5, 2024
23 checks passed
@mart-mihkel mart-mihkel deleted the complete_hyphen branch September 5, 2024 17:41
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.

Support allow_hyphen_values in native completions
3 participants