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

Fix several parsing issues + association of short and long options #20

Merged
merged 2 commits into from Nov 26, 2015

Conversation

toch
Copy link
Contributor

@toch toch commented Sep 26, 2015

Hi,

That patch fixes the following:

  • when an option name between <> contains space, it's not handled correctly
  • long option is detected as a short option
  • default value is not detected
  • when a long and short options are possible, they are not put together

In the last case, the value set for the passed version is also assigned to its companion: for instance, if "-h, --help" are defined, if we pass only "-h", both "-h" and "--help" are set to true. The inverse is also true. I didn't find if it was or not the behavior expected, but I considered as a bug.

I've also updated the test cases to take into account that long and short options are associated.

what do you think of it?

@jaredgrubb
Copy link
Member

I like your regex changes; however, I'm hesitant to accept the behavior change in the tests. The tests actually come from the official Python version of this concept, and changing the tests forks the behavior... which I'm not sure is a good idea. What do you think?

@toch
Copy link
Contributor Author

toch commented Sep 28, 2015

@jaredgrubb Indeed, it makes sense to keep the same behaviour through all implementations.

I wasn't aware the testcases were the same for every implementations. I found it's a pity that long and short options are not linked and you need to check both to get the value. That's why I submitted those changes.

Do you think it would be possible to convince to move the reference testcases in that way?

If not, what about a flag to select the behavior?

If not, I'll remove the part https://github.com/docopt/docopt.cpp/pull/20/files#diff-d114590b6749c3ff46e3b45297225633R1039

by the way, do you want me to squash my commits in one?

@toch
Copy link
Contributor Author

toch commented Oct 13, 2015

Hi, any feedback?

@jaredgrubb
Copy link
Member

Yes, if you remove the test-case changes, and that one part, then I think we can merge this.

* match short and long option when provided
* regexp can recognize if the option is short or not
@toch
Copy link
Contributor Author

toch commented Oct 25, 2015

Hi @jaredgrubb, I did the changes so the test-cases are respected now.

By checking the python version, I understood how long and short options are handled when both possible: it's the long option that it is set. I'm wondering if we shouldn't provide in Option class a method making that easy, something like name. It would return the short version if it's the only one possible, and the long version if both are provided or just the long option. What do you think? I can do a suggestion in another PR.

@toch
Copy link
Contributor Author

toch commented Nov 2, 2015

Hi @jaredgrubb a quick reminder of my last suggestion :)

@toch
Copy link
Contributor Author

toch commented Nov 20, 2015

Hi,

Is there anything else I can do for that PR?

@jaredgrubb
Copy link
Member

Sorry for the delay! I think this looks good. I'll merge this in.

jaredgrubb added a commit that referenced this pull request Nov 26, 2015
Fix several parsing issues + association of short and long options
@jaredgrubb jaredgrubb merged commit aa1c02a into docopt:master Nov 26, 2015
@toch
Copy link
Contributor Author

toch commented Nov 26, 2015

no worries :) and thanks for the review. Happy to have helped.

@toch toch deleted the set-short-long-options branch November 26, 2015 18:01
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.

None yet

2 participants