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

Parser refactor. #2297

Merged
merged 13 commits into from
Feb 7, 2021
Merged

Parser refactor. #2297

merged 13 commits into from
Feb 7, 2021

Conversation

ldm0
Copy link
Member

@ldm0 ldm0 commented Jan 17, 2021

A little parser refactor.

Fixes #1026, check tests/grouped_values.rs for the results.
Fixes #1374, check tests/grouped_values.rs:issue_1374().
Fixes #2171, check tests/grouped_values.rs:issue_2171().

@ldm0
Copy link
Member Author

ldm0 commented Jan 18, 2021

One sad thing is that the clippy warning make CI red is removed in currently nightly clippy but we have to deal with it in 1.46.0. :-P rust-lang/rust-clippy#6313

@ldm0 ldm0 force-pushed the parser branch 6 times, most recently from 0f96e77 to a960852 Compare January 19, 2021 18:32
@ldm0 ldm0 marked this pull request as ready for review January 19, 2021 19:12
@ldm0
Copy link
Member Author

ldm0 commented Jan 20, 2021

This also fixes #2229, add tests later.

Edit:
Nope, will fix this in another patch.

@ldm0 ldm0 force-pushed the parser branch 3 times, most recently from d60981b to 6d52174 Compare January 23, 2021 07:22
@ldm0
Copy link
Member Author

ldm0 commented Jan 23, 2021

Hmm, sorry this PR is a bit long... The reason it because I need to prove that this refactor makes it easy for future bug solving and feature implementing, which I prove it in the last few commits. But it make this PR hard to review. I'll seperate it into several commits later.

@ldm0 ldm0 mentioned this pull request Jan 23, 2021
@ldm0 ldm0 force-pushed the parser branch 2 times, most recently from c45741c to f890ef8 Compare January 23, 2021 14:52
@pksunkara
Copy link
Member

Do you have a solution for #1794 on top of this parser refactor? I don't want to revert the fix without any other fix.

@ldm0
Copy link
Member Author

ldm0 commented Jan 24, 2021

Do you have a solution for #1794 on top of this parser refactor? I don't want to revert the fix without any other fix.

@pksunkara

But that's a fragile fix(I don't think it's a fix actually). I showed you the bad case in that issue comment. The inner detail is that the parser is using a positional counter to determine which positional argument should take current value. And also, the group validation is processed after all parsing is done. This is the reason of #1794, the parser cannot access arg group info in the parsing stage so it puts the value in pos1 even with the --option presented.

Now let's look at how #1856 fix this. It introduces the maybe_inc_pos_counter. It's behaviour is when you see a flag which is in a group with other positional args, increases the positional counter by the number of other positional args.

Do you notice the problem? It just increases the positional counter rather than effectively avoiding the parser pushing value to the sibling positional args or stealing already pushed value from sibling positioal args. Which causes the problem I pointed in the issue comment, just with a little change to the test case, this kind of hacky workaround will fail.

This is the design problem, and won't have an easy fix until the parser have the ability to steal the value from an arg to another, which needs a redesign of our parser. And the problem will only occur when a user groups a flag with positional argument. I don't like reverting fixes either, but I have to :

  1. Current fix is bad, totally fixing it needs a deeper refactor.
  2. Current fix make the code hard to read and change.

Hope you understand...

@pksunkara
Copy link
Member

Okay, but do you have an idea (even though it's not implemented yet) on how to fix it and are going to work it on soon?

@ldm0
Copy link
Member Author

ldm0 commented Jan 24, 2021

Okay, but do you have an idea (even though it's not implemented yet) on how to fix it and are going to work it on soon?

@pksunkara There are several options on fixing this. For example, we can push all of the parsed positional values to a vector and pushing them to each positional arguments at last(there are more details). Another option is validating on each parsing stage, and rollback on parsing failure.

tests/app_settings.rs Outdated Show resolved Hide resolved
@pksunkara
Copy link
Member

I don't think the last commit needs to be in this PR. Please make a separate one once this is merged.

@pksunkara
Copy link
Member

Oh, so first #2320? I thought this was first. Got it.

@ldm0
Copy link
Member Author

ldm0 commented Feb 6, 2021

Ok, these are last bits(hard to be split) @pksunkara

@pksunkara
Copy link
Member

Aren't the last 2 commits for different things?

@ldm0
Copy link
Member Author

ldm0 commented Feb 6, 2021

Aren't the last 2 commits for different things?

It seems that only the last commit is for different thing(previous commits are all doing one thing: grouped values). The last commit is based on previous commits, and also it's acceptable if previous commits is acceptable. So seperating out it is not that meaningful.

Copy link
Member

@pksunkara pksunkara left a comment

Choose a reason for hiding this comment

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

Looks good. I guess users would need grouped_values_of only when they have both MultipleValues and MultipleOccurrences turned on, right?

Maybe we should have a better name for that?

tests/flags.rs Outdated Show resolved Hide resolved
tests/grouped_values.rs Outdated Show resolved Hide resolved
src/parse/matches/arg_matches.rs Show resolved Hide resolved
src/parse/matches/matched_arg.rs Outdated Show resolved Hide resolved
src/parse/matches/matched_arg.rs Outdated Show resolved Hide resolved
src/parse/parser.rs Outdated Show resolved Hide resolved
src/parse/parser.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants