Add a way to create options and arguments for any type that implements IParseable/ISpanParseable #2658
Conversation
|
Strictly speaking none of this is necessary, and thanks to the fact that the Argument and Option types are unsealed this could be done at a layer above S.CL itself. |
| if (arg.Arity.MaximumNumberOfValues == 1 && result.Tokens.Count > 0) | ||
| { | ||
| var text = result.Tokens[result.Tokens.Count - 1].Value; | ||
| if (T.TryParse(text.AsSpan(), CultureInfo.CurrentCulture, out var parsed)) |
There was a problem hiding this comment.
This use of CultureInfo.CurrentCulture perpetuates the #1733 bug, making scripts less reliable as the script author cannot hardcode an argument value and have the tool parse it identically regardless of the user's culture settings.
There was a problem hiding this comment.
Good call-out - these APIs accept null so that should maybe be used instead. Or the invariant culture.
There was a problem hiding this comment.
null would be worse, because it is treated as CultureInfo.CurrentCulture at run time but will not be found when one searches the source code for uses of CultureInfo.CurrentCulture.
There was a problem hiding this comment.
When I was trying to come up with another solution for the exact problem in my wild prototype, I came up with an interface called IParsableSymbol, it was implemented by both Option<T> and Argument<T> but I was still unable to find a clear way of creating options and arguments without using reflection in a way that would be trimmer-friendly, so I came up with static factory methods CreateParsable:
I am sharing it just as a reference of a different approach that was not perfect.
Ok, this is a bit of a vibe-coded fever dream.
I was looking at #2574 and wondering why we had to keep patching this list. IParseable/ISpanParseable have been in the BCL for quite some time, after all. I worked with Copilot to iterate on this, and initially trying to get these interfaces integrated into the existing Argument shapes meant that there would be reflection used - because the argument conversion would try to probe the type to see if it was ISpanParseable and then try to call that member.
Instead, I surfaced subclasses of Option/Argument that encode the conversion routine to use the appropriate TryParse member. At least for single tokens, this seems to work like a charm! For handling multiple tokens, we unfortunately hit the reflection-based paths that exist today even for these new subclasses. But at least there's some statically-verifiable way to do this now!