-
Notifications
You must be signed in to change notification settings - Fork 414
Docs improvements #2709
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
Docs improvements #2709
Conversation
| /// <summary> | ||
| /// The token that was parsed to specify the directive. | ||
| /// Gets the token that was parsed to specify the directive. | ||
| /// </summary> | ||
| public Token Token { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unclear in the case where the command line has multiple directive tokens with the same name: [name] [name:value1] [name:value2]. IIRC the library parses those and collects all of them to the same DirectiveResult instance, whose Values property then is { "name1", "name2" } (not sure whether the null value is just lost entirely). But which token do you get in the Token property then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolving to merge since I don't know the answer. I will unresolve after merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC the library parses those and collects all of them to the same DirectiveResult instance, whose Values property then is { "name1", "name2" }
This is correct.
(not sure whether the null value is just lost entirely). But which token do you get in the Token property then?
This is a bit of a rough edge, and there's a bug. I wrote a quick test to explore this.
- There's no
nullvalue in theValuesproperty (which I think is reasonable but I could see an argument for it either way). - The
Tokenproperty contains the first instance of the directive and theTokensproperty contains the others. This seems like a bug.
|
@adamsitnik Could you review? |
|
Can someone from the team comment on the outstanding questions so I can move forward with this PR? |
adamsitnik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you @gewarren !
src/System.CommandLine/System.Runtime.CompilerServices/Range.cs
Outdated
Show resolved
Hide resolved
src/System.CommandLine/System.Runtime.CompilerServices/Range.cs
Outdated
Show resolved
Hide resolved
|
@jonsequitur Could you merge it please? I don't have perms. |
Contributes to dotnet/dotnet-api-docs#11392.