-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
gh-80010: Expand fromisoformat to include most of ISO-8601 #92177
Conversation
55ee790
to
c79e33f
Compare
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. |
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.
Is there a way to limit this to only the rfc3339 subset? The full set of iso8601 features is not always appropriate. |
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
e51e734
Compare
0b2bffd
to
ea9c3d6
Compare
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.
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
a33d776
Compare
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: