Minor token improvements #2080
Minor token improvements #2080
Conversation
| @@ -46,7 +46,7 @@ internal Token(string? value, TokenType type, Symbol? symbol, int position) | |||
| /// <summary> | |||
| /// The Symbol represented by the token (if any). | |||
| /// </summary> | |||
| internal Symbol? Symbol { get; set; } | |||
| public Symbol? Symbol { get; internal set; } | |||
There was a problem hiding this comment.
We should test that this is correctly reassigned when OnlyTake is used.
| @@ -59,21 +59,5 @@ internal Token(string? value, TokenType type, Symbol? symbol, int position) | |||
|
|
|||
| /// <inheritdoc /> | |||
| public override string ToString() => Value; | |||
|
|
|||
There was a problem hiding this comment.
Standard guidance (https://learn.microsoft.com/en-us/dotnet/api/system.iequatable-1?view=netstandard-2.0#notes-to-implementers) does suggest we should keep the equality operators for consistency, even if we aren't using them at the moment.
From a personal perspective, I like having the equality operators available since it makes the code simpler when dealing with nullable types, and using the equality operators helps avoid (via compilation errors) accidentally making typos that result in calling type1.Equals(type2) (calling into Type1.Equals(object), always returning false).
| using System.CommandLine.Parsing; | ||
| using Xunit; | ||
|
|
||
| namespace System.CommandLine.Tests |
There was a problem hiding this comment.
Are we leveraging file scoped namespaces?
I see at least one file was converted to use them by @jonsequitur in #2226
While creating the API design proposal I've noticed that:
Tokenctor acceptsSymbol, but the property is internal. Some users already asked for exposing in the past (Make Token.Symbol property public #1815)Tokendoes not need to override the quality operators, I have most likely introduced them when I've made it a struct in the past