Expose value and error on parser result #634
Conversation
johnjaylward
force-pushed
the
ExposeValueAndErrorOnParserResult
branch
from
4311b05
to
4bc7803
Compare
johnjaylward
force-pushed
the
ExposeValueAndErrorOnParserResult
branch
from
4bc7803
to
108b208
Compare
src/CommandLine/ParserResult.cs
Outdated
|
|
||
| internal NotParsed(TypeInfo typeInfo, IEnumerable<Error> errors) | ||
| : base(ParserResultType.NotParsed, typeInfo) | ||
| internal NotParsed(IEnumerable<Error> errors, TypeInfo typeInfo) |
There was a problem hiding this comment.
Is there a reason to switch the parameters of ctor? It's better to keep internal API the same.
It also keep tests without change.
There was a problem hiding this comment.
I was trying to be have the API be consistent across the Hierarchy.
ParseResult(value/error, type) matches Parsed(value, type) and NotParsed(error, type)
I thought it made it easier to see the relation between the constructors better, but I can revert it if you prefer having the parameter orders not match.
There was a problem hiding this comment.
I see, but keeping test without change is very necessary. It proves no break change.
There was a problem hiding this comment.
I'm not sure I understand your comment. Are you saying that there may be other places using the "internal" constructor outside of this library? That shouldn't be possible unless someone is doing very weird reflection:
https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/internal
the change should only affect the internals of this library, which includes the test cases.
There was a problem hiding this comment.
Internals are used by other PRs and test suit.
There are some PRs are still open and sure that will effect their work when merged and cause confliction when merging.
There was a problem hiding this comment.
I will revert those changes then.
There was a problem hiding this comment.
Thanks for the good work.
|
Good solution. |
|
@johnjaylward Thank you so much for making this PR! The old flow felt very awkward and this change makes my code cleaner :) |
johnjaylward
force-pushed
the
ExposeValueAndErrorOnParserResult
branch
from
108b208
to
3094bfb
Compare
|
I added the test cases. Let me know if they match your expectations or need more improvements. |
|
Yes, the tests are enough and no breaking change 👍 |
Fixes #543
Fixes #165
@moh-hassan no existing tests break when I run them through visual studio. I have not added any new tests. Do you want to see any specific examples be tested?