- Notifications
You must be signed in to change notification settings - Fork412
Add a way to create options and arguments for any type that implements IParseable/ISpanParseable#2658
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Strictly speaking none of this isnecessary, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This use ofCultureInfo.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
null would be worse, because it is treated asCultureInfo.CurrentCulture at run time but will not be found when one searches the source code for uses ofCultureInfo.CurrentCulture.
adamsitnik left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
When I was trying to come up with another solution for the exact problem in my wild prototype, I came up with an interface calledIParsableSymbol, it was implemented by bothOption<T> andArgument<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 methodsCreateParsable:
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'ssome statically-verifiable way to do this now!