-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Let tryparse accept AbstractChar arguments #47617
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
base: master
Are you sure you want to change the base?
Conversation
This is still good to go. CI failure seems unrelated. |
I've rebased this and updated the first post to better describe the issue. The CI failures still seem like false positives. Could this be flagged for triage, please? |
I'm not sure this needs triage; it needs a reviewer 🙂 making
this is a bit clunky. maybe instead make the first line in the docstring |
I think I might have copied that line of the docstring from I'm happy with your suggestion. Are you happy with the code, besides that docstring change? |
yes, it looks good to me. to be completely proper, we should add a compat annotation like
|
Cool. I think it should be
Cos other types in 3rd party code may define parse or tryparse for other types. I'm happy for you or someone else to male these small changes and merge this PR. |
closes #45640 |
I remember why I wrote it this way now, it's because I could fix that by removing the type restriction. |
FYI, it's possible #59603 might end up subsuming this PR as it also adds this method (+ some performance improvements). sorry to have your thunder stolen 🙂. if you are interested in helping review that it could be helpful. of course if there ends up being no motion there for a while we could merge this anyway |
Cool, well I guess we'll see what happens. I'm not precious about someone else fixing this! |
This PR lets
tryparse
parseAbstractChar
s in the same way thatparse
does.This PR also documents that
parse
accepts characters as well as strings.