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

Add GetoptMode parser setting and implementation #684

Merged
merged 4 commits into from
Aug 25, 2020

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Aug 19, 2020

Turning on Getopt mode automatically turns on the EnableDashDash and AllowMultiInstance settings as well, but they can be disabled by explicitly setting them to false in the parser settings. This is essentially #607 rebased onto the develop branch, but with an option (defaulting to false) to turn it on. If the user doesn't set GetoptMode to true, then no behavior changes.

Note that two unit tests will fail until #682 is merged, because they test for the same bug that #682 fixes.

This PR includes the FlagCounter feature from #607 as well; I've made the FlagCounter feature and the GetoptMode feature two separate commits so that if you want to use one but not the other, it's easy to cherry-pick the one you want to use. They don't depend on each other; in particular, FlagCounter works even without GetoptMode being true. But I've included both of them in this PR because GetoptMode without FlagCounter feels incomplete: people running in Getopt mode are going to expect to be able to do -vvv and get VerboseLevel=3.

My next PR will be a rebasing of #608 onto develop. Once that's done, if I'm sure that this PR is going to be merged, I'll start working on writing documentation of GetoptMode for the CLP wiki. In the meantime, the unit tests in GetoptParserTests.cs are pretty self-documenting.

Fixes #601.
Fixes #631.

I've also fixed #687 in this PR (in commit 47618e0); I chose to incorporate that fix into this PR because GetoptTokenizer, introduced here, contains the same buggy ExplodeOptionList method as the original Tokenizer, and both of them need to be fixed to properly fix #687. In addition to #687, this PR now:

Fixes #619
Fixes #617
Fixes #510
Fixes #454
Fixes #420
Fixes #396
Fixes #91

@rmunn
Copy link
Contributor Author

rmunn commented Aug 19, 2020

The failing unit tests are expected, and are fixed by #682. So #682 should be merged first, and then I'll rebase this PR on top of the new head of the develop branch so that the unit tests will pass.

rmunn added 2 commits August 20, 2020 06:46
FlagCounter lets an int property count the number of times a flag
appears, e.g. "-v -v -v" would produce the value 3 in an int property
decorated with [Option('v', FlagCounter=true)].
Turning on Getopt mode automatically turns on the EnableDashDash and
AllowMultiInstance settings as well, but they can be disabled by
explicitly setting them to false in the parser settings.
@rmunn rmunn force-pushed the feature/getopt-mode branch from c6f1701 to 570d7b7 Compare August 19, 2020 23:47
@rmunn
Copy link
Contributor Author

rmunn commented Aug 19, 2020

Just rebased on top of the new state of develop now that #682 has been merged.

@rmunn
Copy link
Contributor Author

rmunn commented Aug 20, 2020

@moh-hassan - 47618e0 contains a fix for eight different bugs (listed in the PR description above). I've chosen to include it in this PR because GetoptTokenizer.cs (introduced in this PR) had the same bug, and needed the same fix, as the original Tokenizer.cs. (Because the buggy code is a method that I copied over from the original Tokenizer without modifying it). I thought about whether to submit that bugfix as a separate PR, but that would have caused this PR to get rather messy (this PR would then depend on another PR that had to be committed first, and it would have been rather complicated to get them merged in the right order, kind of like what happened with my original #607).

However, if you decide that you do not want to take this PR, please let me know and I'll do the work of separating out the Tokenizer bugfix from the rest of 47618e0 so that that bugfix, at least, can go in. But the simplest way by far is to just include that bugfix in this PR.

@moh-hassan
Copy link
Collaborator

moh-hassan commented Aug 20, 2020

Thanks @rmunn for this Great PR.
I agree for the bugfix included in this PR.
ParserSettings.GetoptMode =true/false (false is default) is used.
To enable future extensibility and avoid breaking change, It's planned to support other Commandline standard like Posix and forward slash in windows and enable the corresponding tokenizer:

  • I suggest use enum like:
enum ParserMode
{
Gnu ,  //default
GetOpt , 
//Posix,   
//SingleDash , //like powershell , Unity engine, msbuild
//ForwardSlash  // windows 
 
}

you can rename the options.

  • Can you provide specialized test cases that are unique to GetOptMode?
  • Can you provide a wiki page how to use GetoptMode with code example?
  • It's nice to share the common code between Tokenizer and GetoptTokenizer for maintenance.

@rmunn
Copy link
Contributor Author

rmunn commented Aug 21, 2020

Hmm. The current parser mode isn't exactly GNU, because GNU getopt is what this PR is about emulating. I'm not quite sure what to call it, though: it's a weird mix of GNU behaviors and non-GNU behaviors. Perhaps "Default" mode would be a good name. And if there's ever a plan to change the default mode in the future (let's say there's widespread demand for Getopt mode to become the default in 3.x), then the current mode can be called "Legacy" mode when that breaking change is made, so that people who upgrade to 3.x but want to keep the 2.x behavior can just change "ParserMode = Default" to "ParserMode = Legacy" and the breaking change only forced them to change one line of code.

Or even better, we call the current mode "Legacy" right from the start, and also have an enum value called Default that is assigned to be the same as "Legacy" right now, but in 3.x might change to be assigned to some other mode. Yeah, I like that. That way people who use the default parser get the default mode, which might change in the future. And people who want to NOT have their current parser mode change in the future when we change the default can set it to "ParserMode = Legacy" right now, and we can guarantee that that won't change.

To answer your questions:

  • This PR already includes specialized GetoptMode test cases, in the GetoptParserTests.cs file. Are they insufficient and you want to see more tests?
  • I plan to work on the Wiki page next. I do want to wait until I know there will be no further changes needed to this PR and it will be merged as-is into the 2.9 release, because if (for example) I had already written up documentation for GetoptMode = true, I would have had to change it. So once you tell me, "Yes, this looks good, I plan to merge this without any more changes", I'll get started on the Wki documentation.
  • The common code may not always stay common... but I guess if that happens we can copy the code from Tokenizer back into GetoptTokenizer and then make changes only to the GetoptTokenizer copy of the no-longer-common code. So I'll go ahead and deduplicate the common code now, and I can reverse that in the future if needed.

* Share common code between Tokenizer and GetoptTokenizer
* Use enum for ParserMode as we might add more in the future
@moh-hassan moh-hassan requested a review from gsscoder August 22, 2020 16:51
@moh-hassan moh-hassan added this to the 2.9 milestone Aug 22, 2020
@moh-hassan moh-hassan requested review from moh-hassan and removed request for gsscoder August 25, 2020 14:06
@moh-hassan
Copy link
Collaborator

@rmunn
The term Legacy may mean it's becoming obsolete and don't address that the library support GNU standard (with customization).
I'll revert the last commit and use Getopt =true/false.

@moh-hassan moh-hassan merged commit b7102d8 into commandlineparser:develop Aug 25, 2020
@moh-hassan
Copy link
Collaborator

@rmunn
Please, can you provide a wiki page for this feature with code example and guideline when to use getopt=true

@rmunn
Copy link
Contributor Author

rmunn commented Aug 26, 2020

@moh-hassan There were some documentation improvements in that commit as well as the change to using an enum for ParserMode, so I'll submit a new PR with just those documentation changes so that they don't get lost by your reverting commit b7102d8.

BTW, if you decided to revert b7102d8, does that mean that you're giving up on the idea of a ParserMode enum in the future that would allow SingleDash or ForwardSlash options? Because if we release 2.9 with GetoptMode as a boolean, that will mean that we can't easily change to the ParserMode system in the future without breaking backwards compatibility for everyone who was using GetoptMode = true in their code. So instead of just reverting b7102d8, I wish you had just edited it to change the name Legacy to something you liked better. I picked Legacy because I think people will want Getopt mode to become the default in the future, but you could have easily edited it to call it Gnu if that's your preference. I think reverting that commit was a mistake, but as long as we fix that mistake before pushing the 2.9 release out the door, it won't become TOO big a problem. If we push 2.9 out the door as it stands now, then you're going to have backwards compatibility problems when you finally create the ParserMode enum that you asked for in #684 (comment).

@rmunn
Copy link
Contributor Author

rmunn commented Aug 26, 2020

@moh-hassan BTW, I'm going to start working on a Wiki page now.

@moh-hassan
Copy link
Collaborator

@rmunn
The commit b7102d8 can be restored with changing the options of enum
Do you agree for the next names?

CustomGetOpt  //default
GetOpt

@rmunn
Copy link
Contributor Author

rmunn commented Aug 27, 2020

Yes, CustomGetopt would work. (The choices should be spelled Getopt and CustomGetopt, not GetOpt and CustomGetOpt, by the way: see here, for example, where the GNU documentation capitalizes the word getopt as Getopt, not as GetOpt).

@rmunn
Copy link
Contributor Author

rmunn commented Aug 27, 2020

@moh-hassan Forgot to tag you on my reply, so here's a tag so you get a notification. As I said, the choices should be spelled CustomGetopt and Getopt (with a lowercase o, not uppercase O) but apart from that I'm okay with the name CustomGetopt instead of Legacy. It will need documentation explaining the difference between Getopt and CustomGetopt, but that's precisely what my wiki page is going to be. I'll follow the steps outlined in #486 and submit patches. (Though if I'm going to be doing this a lot, I may end up asking for write access to the repo so that I can edit the wiki more directly and not have to bug you every time. But for now, I'll just submit a PR with my wiki edits).

@rmunn
Copy link
Contributor Author

rmunn commented Aug 27, 2020

@moh-hassan - I have an even better idea. Instead of calling the current mode CustomGetopt or Legacy, we should call it Classic mode. Unlike Legacy, that doesn't imply that the mode is old or deprecated. Rather, calling it Classic mode implies that: 1) this is the current behavior, and 2) this is a mode that will be supported long into the future even if we change the default behavior at some point. So instead of calling it CustomGetopt, when you put commit b7102d8 back in, please call it Classic mode so that there will be less confusion in the long run.

@rmunn
Copy link
Contributor Author

rmunn commented Aug 27, 2020

@moh-hassan I've opened #690 so that you can just merge that in easily if you are happy with the name Classic.

@moh-hassan
Copy link
Collaborator

moh-hassan commented Aug 27, 2020

@rmunn
Thanks for the suggestion.
It's better for the name to express the standard "Getopt" it implement, so developer don't confuse when set Parsermode to Classic.
CustomGetopt means Tokenize with customization: e.g. 'AllowDashDash', 'IgnoreUnknowArguments', ...
So,kindly, I'll merge the PR #690 with the name "CustomGetopt" (with small letter o :)".

@rmunn
Copy link
Contributor Author

rmunn commented Aug 28, 2020

@moh-hassan Sigh. That's a very bad decision that will just cause lots and lots of user confusion, but I can't stop you. The Getopt mode also allows the same amount of customization (you can turn AllowDashDash off if you want, IgnoreUnknownArguments works as designed, and so on), so haivng a CustomGetopt mode and a Getopt mode is going to do nothing but confuse people. I'm working on Wiki documentation explaining in detail the difference between Classic mode and Getopt mode, and I can certainly rename Classic to CustomGetopt in that documentation if you do carry through with that bad decision. But I again URGE you to reconsider and not make the mistake of calling these two modes something so similar that people will constantly get them confused. That's a mistake that will haunt the library forever if you make it now. Please don't do it.

P.S. Earlier I was okay with the name CustomGetopt, but that was before I started trying to explain the difference in documentation. Once I started to try to explain the difference I realized that the name CustomGetopt tells the user nothing about how it differs from Getopt, and so the similarity in the names is just going to confuse people for no reason. That's when I decided to call it Legacy, but Classic would have been even better which is why I went with Classic for #690.

@moh-hassan
Copy link
Collaborator

@rmunn

What about these options:

GetoptParserV1   //default
GetoptParserV2

Both implement Getopt and no confusion .

@derrickstolee
Copy link

Hi, @rmunn. I'm interested in consuming a version of your package with this fix in https://github.com/microsoft/scalar. Do you have a timeline on the 2.9 release?

rmunn added a commit to rmunn/commandline that referenced this pull request Feb 6, 2021
After discussion in PR commandlineparser#684, renamed ParserMode enums to GetoptParserV1
and GetoptParserV2, so as to not communicate the idea that the older
mode is in any way obsolete (it will continue to be supported for the
foreseeable future).
@rmunn
Copy link
Contributor Author

rmunn commented Feb 6, 2021

@moh-hassan I have finally found time to work on CommandLineParser again. I like the GetoptParserV1 and V2 names you suggested in #684 (comment), and I've implemented that change in #690. I'm also working on a wiki page explaining the difference between GetoptParserV1 and V2, and I'll submit it soon. Meanwhile, I think it might be a good idea to merge #690 before the 2.9 release goes out, so that the parser mode can be an enum instead of boolean GetoptMode value — this will allow for future expansion in the parser modes, such as the "use / as the option character" mode that people from Windows backgrounds often ask for.

@rmunn
Copy link
Contributor Author

rmunn commented Feb 6, 2021

@derrickstolee The 2.9 release was held up by the old NuGet API key for the package having expired. #733 updates the NuGet API key, so once that gets merged the release will be possible. Watch #733 if you want to find out when that happens. Unfortunately, I can't do anything about the release timing, as I'm not one of the project maintainers. I believe @ericnewton76 is the one who manages releases right now; Eric, please correct me if that's mistaken.

@rmunn
Copy link
Contributor Author

rmunn commented Feb 6, 2021

#735 and #709 are also fixed by this PR, and should be closed once 2.9.0 is finally released.

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.

3 participants