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

.conflicts_with error messages not symmetric #718

Closed
joshtriplett opened this issue Oct 29, 2016 · 5 comments
Closed

.conflicts_with error messages not symmetric #718

joshtriplett opened this issue Oct 29, 2016 · 5 comments
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-enhancement Category: Raise on the bar on expectations

Comments

@joshtriplett
Copy link
Contributor

The documentation for conflicts_with says:

Conflict rules only need to be set for one of the two arguments, they do not need to be set for each.

NOTE: Defining a conflict is two-way, but does not need to defined for both arguments (i.e. if A conflicts with B, defining A.conflicts_with(B) is sufficient. You do not need need to also do B.conflicts_with(A))

However, the error messages for the conflict differ based on whether I provide one conflicts_with or both.

If I mark the arguments --rfc and --subject-prefix as each conflicting with each other, then passing both will give an error message that treats the second such argument as the error and suggest removing it, which makes sense:

(1) ~/src/git-series$ git series format --rfc --subject-prefix x
error: The argument '--subject-prefix <Subject-Prefix>' cannot be used with '--rfc'

USAGE:
    git series format --rfc

For more information try --help
(1) ~/src/git-series$ git series format --subject-prefix x --rfc 
error: The argument '--rfc' cannot be used with '--subject-prefix <Subject-Prefix>'

USAGE:
    git series format --subject-prefix <Subject-Prefix>

For more information try --help

However, if I only call .conflicts_with("subject-prefix") on --rfc, but not the other way around, the error message always marks --subject-prefix as the error and always suggests dropping --subject-prefix:

(1) ~/src/git-series$ git series format --rfc --subject-prefix x
error: The argument '--subject-prefix <Subject-Prefix>' cannot be used with '--rfc'

USAGE:
    git series format --rfc

For more information try --help
(1) ~/src/git-series$ git series format --subject-prefix x --rfc 
error: The argument '--subject-prefix <Subject-Prefix>' cannot be used with '--rfc'

USAGE:
    git series format --rfc

For more information try --help
@kbknapp
Copy link
Member

kbknapp commented Oct 29, 2016

Thanks for posting this. Yeah it has to do with the order in which clap detects the conflict, and then searches for the root cause of the error. I think it's because clap uses an early exit strategy, and returns an error as soon as it comes to it, instead of parsing the whole line.

For your first example, the way it works is:

  • clap detects --rfc and adds --subject-prefix to list of known conflicts
  • Then detects --subject-prefix and exits with a known conflict error

For the second example it goes a little differently:

  • clap detects --rfc
  • clap detects --subject-prefix and adds --rfc to a list of known conflicts`
  • clap finishes parsing the rest of the arguments
  • Prior to returning a success, it does a once over all the matches, and ensures there are no conflicts
  • clap finds --rfc is on the known conflict list and searches for the a used argument that conflicts with it
  • clap finds --subject-prefix and exits with a conflict error, but doesn't actually know which one came first

But since what you're posing makes 100% sense, I do need to look at a way to make this more symmetric and less confusing.

@kbknapp kbknapp added C-enhancement Category: Raise on the bar on expectations P4: nice to have A-parsing Area: Parser's logic and needs it changed somehow. labels Oct 29, 2016
@joshtriplett
Copy link
Contributor Author

@kbknapp You could store all conflicts symmetrically, manufacturing and adding to a conflict group. For instance, when you call conflicts_with from one arg to another, it'd look up whether that arg already has a conflict group, add itself to that conflict group if so, or create a new conflict group with both arguments if not.

@kbknapp
Copy link
Member

kbknapp commented Oct 30, 2016

Yeah, I'm going to look at some of those possibilities. I'll first have to get re-aquainted with the code since it's been a while since I looked at that piece 😜 I know some easy ways to do it, but I'd also like to keep from allocating more than I absolutely need to.

@kbknapp
Copy link
Member

kbknapp commented Oct 31, 2016

@joshtriplett I was able to get this working. Once #720 merges let me know if v2.16.4 is more along the lines of what you're thinking.

@joshtriplett
Copy link
Contributor Author

Looks promising. I look forward to testing it.

@homu homu closed this as completed in 3d37001 Oct 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-enhancement Category: Raise on the bar on expectations
Projects
None yet
Development

No branches or pull requests

2 participants