introduce ParseResult.GetValue<T>(string name) by adamsitnik · Pull Request #2083 · dotnet/command-line-api

adamsitnik · 2023-03-08T22:04:44Z

fixes #2052

adamsitnik · 2023-03-08T22:05:40Z

src/System.CommandLine/Parsing/CommandResult.cs

+
+                    if (!argument.HasDefaultValue && argument.Arity.MinimumNumberOfValues > 0)
+                    {
+                        argumentResult.AddError(LocalizationResources.RequiredArgumentMissing(argumentResult));


this fixes the issue I've mentioned in #2082 (comment) : when a required argument is missing, we report the error for the ArgumentResult, not for CommandResult

adamsitnik · 2023-03-08T22:06:59Z

src/System.CommandLine/ParseResult.cs

            => RootCommandResult.GetValue(option);

+        /// <summary>
+        /// Gets the parsed or default value for the specified symbol name, in the context of parsed command.


this is important: the implementation provides only values for the parsed command, not for the entire symbol tree.

jonsequitur · 2023-03-09T00:12:51Z

src/System.CommandLine.Tests/GetValueByNameParserTests.cs

+            => parseResult.GetValue<T>(argument.Name);
+
+        [Fact]
+        public void In_case_of_argument_name_conflict_the_value_which_belongs_to_the_last_parsed_command_is_returned()


A few additional test cases come to mind:

What happens when an option and an argument name are the same...

at the same level of the tree?

at different levels of the tree?

What happens when symbols with the same name exist at different levels of the tree and the inner one is not provided? Does it fall back to the outer one's value?

Does this vary depending on whether the inner one has a default value defined?

How do customer parsers behave when...

...parsing an inner symbol -x and looking up a value for -x when there's an outer symbol -x?

...parsing an outer symbol -x and looking up a value for -x when there's an inner symbol -x?

...parsing symbol -a and looking up a value for symbol -x...

...which is above it in the tree

...which is below it in the tree

I have opinions on what some of these behaviors should be but I'll wait so I don't anchor anyone.

What happens when an option and an argument name are the same at the same level of the tree?

We should throw.

What happens when an option and an argument name are the same at different levels of the tree?

The new methods provides only values for the parsed command (parseResult.CommandResult), not for the entire symbol tree.

What happens when symbols with the same name exist at different levels of the tree and the inner one is not provided? Does it fall back to the outer one's value?

The outer value will never be returned, only inner if provided or has default.

How do customer parsers behave when...

Such scenario is not supported by design (nobody asked for it), the method is exposed only for ParseResult, not for SymbolResult. It simplifies the design a lot (the cache can be populated just once).

jonsequitur · 2023-03-09T20:19:27Z

src/System.CommandLine.Tests/GetValueByNameParserTests.cs


            getRequired
                .Should()
                .Throw<InvalidOperationException>()


ArgumentException might be more appropriate here, though I could make an argument for InvalidOperationException if the name exists on an unparsed branch of the larger grammar, e.g. under a subcommand that wasn't present in the current result.

jonsequitur · 2023-03-09T20:21:19Z

src/System.CommandLine.Tests/GetValueByNameParserTests.cs

+            getConflicted
+                .Should()
+                .Throw<NotSupportedException>()
+                .Where(ex => ex.Message == $"More than one symbol uses name \"{sameName}\" for command \"{command.Name}\".");


I feel like we should be detecting this at parser configuration time, e.g. under ThrowIfInvalid or earlier.

The name is immutable now, so we should be able to throw at the moment when given symbol is being added to the Command. We should discuss when is the right moment for throwing at our next design meeting.

jonsequitur · 2023-03-09T20:23:42Z

src/System.CommandLine.Tests/GetValueByNameParserTests.cs

+            parseResult.GetValue<int>(sameName).Should().Be(456);
+
+            parseResult = command.Parse($"outer 123");
+            parseResult.GetValue<int>(sameName).Should().Be(123);


Not to reopen the old late binding can of worms, but are there tests for what happens in various cases if GetValue<T> is called for Option<U>?

Great catch, I've added the missing tests and fixed the discovered issue.

jonsequitur · 2023-03-09T20:25:23Z

src/System.CommandLine/Parsing/CommandResult.cs

-                        SymbolResultTree.TryGetValue(Command.Arguments[i], out SymbolResult? parsedResult);
-                        cache.Add(Command.Arguments[i].Name, parsedResult);
-                    }
+                    Populate(cache, Command.Arguments);


If I'm understanding this correctly, a call to Populate could be triggered by a custom parser before the complete command line has been parsed. Will symbols parsed afterward be findable by name?

A custom parser gets access to SymbolResult-derived type, it does not provide a reference to ParseResult. The CommandResult.GetValue<T>(string name) is internal, so it's also impossible to call outside of S.CL before the parsing has been finished. I can move this method to ParseResult itself to avoid confusion.

Edit: I've moved the implementation for ParseResult.

it's also impossible to call outside of S.CL before the parsing has been finished

The specific call I was wondering about is calls to ArgumentResult from within a custom parse delegate. This happens before the ParseResult is ready.

With my latest changes it's impossible

* add test coverage for casting T to U, fix discovered issue * throw ArgumentException instead InvalidOperationException when no symbol is found for given name * move the method from CommandResult to ParseResult to make it clear that it's available only after parsing has finished

adamsitnik added 3 commits March 7, 2023 21:17

add the method so writing tests is possible

0a091b0

implement tests, mostly by extending existing GetValue(Symbol) tests

8794b20

implement first version

df2c8e3

adamsitnik requested review from Keboo and jonsequitur March 8, 2023 22:04

adamsitnik commented Mar 8, 2023

View reviewed changes

address code review feedback, handle required arguments properly

8a0bb9c

adamsitnik force-pushed the getValueByName branch from 1374d11 to 8a0bb9c Compare March 8, 2023 22:09

jonsequitur reviewed Mar 9, 2023

View reviewed changes

add missing tests, throw NotSupportedException for duplicates

4fed962

jonsequitur reviewed Mar 9, 2023

View reviewed changes

adamsitnik force-pushed the getValueByName branch from 0e6425f to 82f24bd Compare March 13, 2023 14:07

jonsequitur approved these changes Mar 15, 2023

View reviewed changes

adamsitnik merged commit 42c5853 into dotnet:main Mar 15, 2023

adamsitnik deleted the getValueByName branch March 15, 2023 18:24

KalleOlaviNiemitalo mentioned this pull request Apr 3, 2023

Accessing global options #1695

Open

Conversation

adamsitnik commented Mar 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsitnik Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsitnik Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonsequitur Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adamsitnik Mar 13, 2023 •

edited

Loading

adamsitnik Mar 13, 2023 •

edited

Loading

jonsequitur Mar 13, 2023 •

edited

Loading