-
Notifications
You must be signed in to change notification settings - Fork 404
fix #2591 #2644
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 #2591 #2644
Conversation
I have two programs that customize help by searching for the automatically-added HelpOption and assigning an instance of a custom action class to its Action property. This PR looks like it would no longer suppress syntax errors when such customized help is invoked. As HelpAction is a sealed class, I wouldn't be able to fix this by deriving my custom action class from HelpAction, either. |
Maybe |
Unsealing HelpAction would allow custom help to ignores parse errors again, but only as a synchronous action, not asynchronous. I wonder whether there is any reasonable scenario that would need an asynchronous action for custom For |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gonna approve this, but I'd ideally like @jeffhandley's thoughts as well.
I'm having a hard time finding a solution that:
- enables replacing the built-in HelpAction entirely (like we've been enabling for the past several changes around helpbuilder, etc)
- doesn't rely on type-testing a known set of types
- doesn't rely on subclassing HelpAction itself (because then folks wouldn't be able to subclass)
- isn't some kind of publically-visible property on CommandLineAction
I think if we can add a test to pin the invariant that the value of this property on the primary action (the one to be invoked) is the only one that changes the behavior of the pipeline, this is good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am very hesitant about this change:
- it's late: BCL is now in ask mode (need to get M2 approval to merge a new API to main)
- it was not reviewed by the API board
- it's quite niche and specific
I know that what we have for special-casing Help as of now sucks:
command-line-api/src/System.CommandLine/Parsing/ParseOperation.cs
Lines 200 to 203 in 7b20621
if (option is HelpOption) | |
{ | |
_isHelpRequested = true; | |
} |
Since this has been documented as "all you need to customize help is to replace HelpOption aciton" this PR would break that as the users would need to also change the value of the new property to make sure their custom help works the same way.
Since it's late in the release cycle I would prefer to avoid introducing any new public APIs and keep the fix small (even if ugly). Then gather more data (perhaps we don't need a boolean flag, but more flexible mechanism?) and introduce new public API after 10 is shipped.
This fixes #2591.
The approach I've taken to this fix generalizes how certain actions interact with parse errors, adding the (currently internal)
CommandLineAction.IgnoresParseErrors
property and removing the special case for help.