Make UnrecognizedArgumentHandling per command scope #371
Conversation
|
This PR is to reproduce #370. |
scott-xu
changed the title
Currently the setting `OptionNameValueSeparators` in sub command does not take effect, which is the same with `UnrecognizedArgumentHandling`. This commit fixed the issue.
|
/azp run all |
|
No pipelines are associated with this pull request. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -31,12 +30,10 @@ private CommandLineApplication _currentCommand | |||
|
|
|||
| public CommandLineProcessor( | |||
| CommandLineApplication command, | |||
| ParserConfig config, | |||
There was a problem hiding this comment.
With this change, is there any point to keeping the ParserConfig class?
It seems this is sort of going the opposite direction I was hoping to go in #298. I was hoping to extract the parsing behavior of CommandLineApplication into a separate class completely.
There was a problem hiding this comment.
If we want to make the parser config per command, we have to associate them. If we want to inherit the config from parent command, we can’t do it inside ParserConfig as it has no info of its parent.
There was a problem hiding this comment.
make the parser config per command
That's probably a bigger refactor than we need right now.
With this PR as is right now, does the ParserConfig class do anything? Or can it be deleted?
There was a problem hiding this comment.
The ParserConfig is still needed. It is a property of CommandLineApplication. It tells the CommandLineApplication that each config is set or not.
There was a problem hiding this comment.
Sure we can move the two properties of ParserConfig to CommandLineApplication as private fields and then delete ParserConfig.
There was a problem hiding this comment.
I understand that you want to avoid "big class". However, the UnrecognizedArgumentHandling and OptionNameValueSeparators are already published API about CommandLineApplication. It will be a breaking change if we remove it from CommandLineApplication.
As I said in #370 (comment), this PR has no open API change.
Fix #370