Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-80010: Expand fromisoformat to include most of ISO-8601#92177
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
pganssle commented May 2, 2022 • 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.
ghost commented May 2, 2022 • edited by ghost
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by ghost
Uh oh!
There was an error while loading. Please reload this page.
55ee790 to c79e33fComparepganssle commented May 2, 2022
I've added an explicit test matrix that doesn't rely on the hypothesis stubs. Feel free to ignore them as part of the review. |
JelleZijlstra 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.
A few comments, I'll have to study the C code more closely.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
orent commented May 3, 2022
Is there a way to limit this to only the rfc3339 subset? The full set of iso8601 features is not always appropriate. |
pganssle commented May 3, 2022
This sort of thing is the reason we delayed this expansion — the scope can easily grow if we do not constrain it to be something very specific, and we couldn't come up with a good way to allow users to express their preferences about what would be parsed. The approach we settled on is that I don't think it would be completely absurd to have a |
f83a340 to e51e734Compare0b2bffd to ea9c3d6CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
jerub 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.
Thanks for asking me to review this change. I've made a few minor notes.
Great to see that we're building these batteries included. It would also be good to have a really solid RFC3339 parser as well. ISO 8601 has the mindshare because of the name, but RFC 3339 is the quality standard that people actually want.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
orent commented May 4, 2022
Instead of adding yet another method - how about making the constructor accept a string? Many other builtin types have a constructor that accepts a single string argument and round-trips their __str__ (which may or may not be the same as their __repr__). So my suggestion is for the constructor to accept the __str__ which is basically rfc3339 (possibly with a different separator) and the .fromisoformat() method would accept the larger set of ISO8601 values. |
8cbe22e to a33d776Comparepganssle commented May 6, 2022
Ok, I think we're down to just nits and other things that can easily be taken care of after feature freeze. I'm going to merge this. |
This should cover all of ISO-8601 except for fractional non-second components.
@godlygeek Would you mind taking a look?
Note: Currently the tests are mostly written as hypothesis tests using the stubs from #22863. Before merge we can try to refactor those out into a big matrix of examples, but for now it's useful to know that we have good coverage of the enormous state space here.
#80010
To Do: