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

ParserMode setting as enum rather than GetoptMode as bool #690

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Aug 27, 2020

This is b7102d8 with the Legacy name renamed to Classic so as not to suggest that it's obsolete or going away anytime soon. See #684 (comment) for the rationale.

This allows adding other modes in the future, while maintaining
backwards compatibility as long as the default mode isn't changed.
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

Enum names are now GetoptModeV1 and GetoptModeV2 as per discussion with @moh-hassan (see #684 (comment) and previous comments for background).

@rmunn
Copy link
Contributor Author

rmunn commented May 13, 2022

@ericnewton76 - Now that there's some motion on the CommandLineParser project again, I'd like to ask you to take a look at this PR. Having GetOptMode as a boolean limits future design choices, and I think it should be an enum instead. Rationale in #684 (comment). In #684 (comment), @moh-hassan expressed an intention to merge this PR with the name "CustomGetopt"; I disagreed with the name, and as you can see, this PR was never merged.

At this point, changing the bool to an enum might be a breaking change, but it's a breaking change in a feature that was only just released so right now would be the time to make it. The longer that change waits, the more people will be affected by it. So please take a look at this PR (#690) as soon as you can. Thanks.

@psfinaki
Copy link

psfinaki commented Jun 7, 2022

Anyone here? Please? @moh-hassan @ericnewton76?

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.

2 participants