Fix duplicate errors appended #2416
Conversation
Fixes dotnet#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.
|
I'd like to wait to approve this PR until we get the Also, I do not understand the change to 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) |
| @@ -31,10 +31,11 @@ public ValueResult ValueResult | |||
| { | |||
| // This is not lazy on the assumption that almost everything the user enters will be used, and ArgumentResult is no longer used for defaults | |||
| // TODO: Make sure errors are added | |||
| var conversionValue = GetArgumentConversionResult().Value; | |||
| ArgumentConversionResult argumentConversionValue = GetArgumentConversionResult(); | |||
There was a problem hiding this comment.
Nit: Mentioning partially because we want to build some guidelines here.
Since the name of the method is GetArgumentConversionResult, I think we should use var.
Thoughts
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.