Skip to content

SymbolResults improvements #2024

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 19, 2023
Merged

SymbolResults improvements #2024

merged 5 commits into from
Jan 19, 2023

Conversation

adamsitnik
Copy link
Member

SymbolResults improvements based on feedback from our last meeting (notes):

  • single, readonly LocalizationResources for entire symbol tree
  • removed Symbol from SymbolResult, as each derived type defines a more specific property (OptionResult.Option etc)
  • moved SymbolResult.Children to Command.Children, as it's the only type that officially has them (the fact that Option has them is an implementation detail)
  • removed a lot of allocations by not storing a list of children per symbol/command and re-using the information present in symbol dictionary (we just search for instances with Parent equal to given symbol result)

I still need to implement a copy-free approach for Symbol.Tokens and I don't like the fact that many of the SymbolResults properties/methods are virtual and just calling Root.MethodAbc. I'll send another PR tomorrow or just update this one.


if (optionNode.Children is null) // no Arguments
{
if (optionResult.Option.Argument.HasCustomParser)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously this logic was performed as part of the validation (ValidateAndConvertOptionResult). Since here we know that given OptionNode was parsed with no arguments, we can create the argument result now and let validation method just validate without performing mutations.

if (optionResult.Children.Count == 0)
{
if (optionResult.Option.Argument is { HasCustomParser: true })
{
if (optionResult.Option is { } opt)
{
var argResult = optionResult.GetOrCreateDefaultArgumentResult(opt.Argument);
optionResult.AddChild(argResult);
ValidateAndConvertArgumentResult(argResult);
}
}
}

{
return;
}
OptionResult optionResult = (OptionResult)_symbolResults[argumentNode.ParentOptionNode.Option];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is guaranteed to work, there can be no parsed OptionArgumentNode without option being parsed and handled first

.Should()
.Be(command);
.BeAssignableTo<CommandResult>()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can use BeOfType here, no?

Copy link
Member Author

@adamsitnik adamsitnik Jan 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iirc it's RootCommandResult here (which is internal)

edit:

[xUnit.net 00:00:00.93]     System.CommandLine.Tests.ArgumentTests+CustomParsing.Command_ArgumentResult_Parent_is_set_correctly_when_token_is_implicit [FAIL]
  Failed System.CommandLine.Tests.ArgumentTests+CustomParsing.Command_ArgumentResult_Parent_is_set_correctly_when_token_is_implicit [34 ms]
  Error Message:
   Expected type to be System.CommandLine.Parsing.CommandResult, but found System.CommandLine.Parsing.RootCommandResult.
  Stack Trace:
     at FluentAssertions.Execution.XUnit2TestFramework.Throw(String message)
   at FluentAssertions.Execution.TestFrameworkProvider.Throw(String message)
   at FluentAssertions.Execution.DefaultAssertionStrategy.HandleFailure(String message)
   at FluentAssertions.Execution.AssertionScope.FailWith(Func`1 failReasonFunc)
   at FluentAssertions.Execution.AssertionScope.FailWith(Func`1 failReasonFunc)
   at FluentAssertions.Execution.AssertionScope.FailWith(String message, Object[] args)
   at FluentAssertions.Types.TypeAssertions.Be(Type expected, String because, Object[] becauseArgs)
   at FluentAssertions.Primitives.ReferenceTypeAssertions`2.BeOfType(Type expectedType, String because, Object[] becauseArgs)
   at FluentAssertions.Primitives.ReferenceTypeAssertions`2.BeOfType[T](String because, Object[] becauseArgs)
   at System.CommandLine.Tests.ArgumentTests.CustomParsing.Command_ArgumentResult_Parent_is_set_correctly_when_token_is_implicit() in D:\projects\command-line-api\src\System.CommandLine.Tests\ArgumentTests.cs:line 340
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

@adamsitnik adamsitnik merged commit 01aee74 into dotnet:main Jan 19, 2023
@adamsitnik adamsitnik deleted the parsingCont branch January 19, 2023 10:59
@adamsitnik
Copy link
Member Author

@jonsequitur thank you for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants