Support for ValueResult as public type to carry value and location #2372
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
KathleenDollard
commented
Apr 5, 2024
| @@ -1,7 +1,9 @@ | |||
| // Copyright (c) .NET Foundation and contributors. All rights reserved. | |||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | |||
|
|
|||
| using System.CommandLine.Directives; | |||
There was a problem hiding this comment.
This is an example of my confusion. If I look at dotnet:main-powderhouse, I see this line present, as well as the files DiagramSubsystemTests.cs, etc.
KathleenDollard
force-pushed
the
powderhouse-valueresult
branch
2 times, most recently
from
ba99c86 to
9d83bd3
Compare
OptionResult and ArgumentResult access internal data and ValueResult is a public face for the data and location.
KathleenDollard
force-pushed
the
powderhouse-valueresult
branch
from
9d83bd3 to
204a997
Compare
KathleenDollard
commented
Apr 5, 2024
| new CliArgument<string[]>("arg2") | ||
| } | ||
| }; | ||
| var command = new CliCommand( |
There was a problem hiding this comment.
Remove newline
| } | ||
| }; | ||
|
|
||
| ParseResult result = CliParser.Parse(command, new[] { "the-command", |
There was a problem hiding this comment.
Check whitespace
| } | ||
| */ | ||
| */ | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove blank line
| @@ -23,6 +23,7 @@ public void The_tokenizer_can_handle_single_option() | |||
| List<CliToken> tokens = null; | |||
| List<string> errors = null; | |||
| Tokenizer.Tokenize(args, command, new CliConfiguration(command), true, out tokens, out errors); | |||
| Tokenizer.Tokenize(args, command, new CliConfiguration(command), true, out tokens, out errors); | |||
There was a problem hiding this comment.
Is this line really doubled?
| @@ -44,6 +47,7 @@ public sealed class ParseResult | |||
| Configuration = configuration; | |||
| _rootCommandResult = rootCommandResult; | |||
| CommandResult = commandResult; | |||
| this.valueResultDictionary = valueResults; | |||
There was a problem hiding this comment.
Probably do not need this this
| @@ -159,6 +165,9 @@ public IReadOnlyList<string> UnmatchedTokens | |||
| public override string ToString() => ParseDiagramAction.Diagram(this).ToString(); | |||
| */ | |||
|
|
|||
| public ValueResult? GetValueResult(CliSymbol symbol) | |||
There was a problem hiding this comment.
Consider TryGetValue here
| /// <summary> | ||
| /// The argument to which the result applies. | ||
| /// </summary> | ||
| public CliArgument Argument { get; } | ||
|
|
||
| internal bool ArgumentLimitReached => Argument.Arity.MaximumNumberOfValues == (_tokens?.Count ?? 0); | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove blank line
| // Copyright (c) .NET Foundation and contributors. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| using System; |
There was a problem hiding this comment.
Clean up usings
| var dict = new Dictionary<CliSymbol, ValueResult>(); | ||
| foreach (KeyValuePair<CliSymbol, SymbolResult> pair in this) | ||
| { | ||
| var result = pair.Value; |
| @@ -0,0 +1,17 @@ | |||
| // Copyright (c) .NET Foundation and contributors. All rights reserved. | |||
There was a problem hiding this comment.
Consider adding this code to ArgumentResult
Also, be sure the ValueResultOutcome method is used
KathleenDollard
changed the title
KathleenDollard
force-pushed
the
powderhouse-valueresult
branch
2 times, most recently
from
20849d6 to
5d1e815
Compare
KathleenDollard
force-pushed
the
powderhouse-valueresult
branch
2 times, most recently
from
ee98942 to
67fc72a
Compare
The exe name appears to differ in CI
KathleenDollard
force-pushed
the
powderhouse-valueresult
branch
from
67fc72a to
9ae597c
Compare
mhutch
approved these changes
Apr 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I am having an issue where the base of this merge is not what I am expecting. You can see the problem in System.CommandLine.Subsystems/Directives/DirectiveSubsystem.cs which exists in dotnet/command-line-api, but appears as completely fresh code in this PR. Another example is that ParserTests.cs has a bunch of whitespace fixes made by @mhutch that do not appear in the base of this PR.
I either did something dumb or I have lost my mind.
Commits in this PR also need to be cleaned up, so it is not ready to go, but I wanted to put this out so I could get help resolving this.