search

dotnet / command-line-api

Command line parsing, invocation, and rendering of terminal output.
https://github.com/dotnet/command-line-api/wiki
MIT License
3.34k stars 375 forks source link

Fix duplicate errors appended #2416

Open Keboo opened 2 months ago

Keboo commented 2 months ago

Fixes #2392. This is a port from the fix on main. However due to additional work that is currently being done on Powerderhouse it is forcing the creation of the ArgumentConversionResult as part of parsing. This is due to the invocation SymbolResultTree.GetValueResultDictionary() which forces the evaluation of all ArgumentConversionResults. This is adding an additional test to make sure that the behavior is not re-introduced. This also fixes a few NRT issues and a comment typo.

KathleenDollard commented 2 months ago

I'd like to wait to approve this PR until we get the ValueSubsystem PR in, as it seems easier to reapply this one.

Also, I do not understand the change to ValueResult. The double call to GetArgumentConversionResult should not have caused any extra work as the result appears cached in a field and used by GetArgumentConversionResult.

I do not object to these changes, just want to understand this. I find this corner of the existing code confusing, at least in naming, and hope we can unwind some of that complexity after we have all the tests reinstated.

The other overall comment: I anticipated more changes in tests. Did we change the test where we discovered this problem in a different PR? (The test that had the false positive in relation to its name)