Skip to content
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

Merged
merged 42 commits into from
May 6, 2022

Conversation

pganssle
Copy link
Member

@pganssle pganssle commented May 2, 2022

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:

  • Add news entry
  • Update docstrings
  • What's new entry

@pganssle pganssle requested a review from abalkin as a code owner May 2, 2022 18:53
@cpython-cla-bot
Copy link

cpython-cla-bot bot commented May 2, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@pganssle
Copy link
Member Author

pganssle 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.

Copy link
Member

@JelleZijlstra JelleZijlstra left a 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.

Lib/datetime.py Outdated Show resolved Hide resolved
Lib/datetime.py Outdated Show resolved Hide resolved
Lib/datetime.py Outdated Show resolved Hide resolved
Lib/datetime.py Outdated Show resolved Hide resolved
Lib/test/datetimetester.py Show resolved Hide resolved
Modules/_datetimemodule.c Show resolved Hide resolved
isoformatter.py Outdated Show resolved Hide resolved
@serhiy-storchaka serhiy-storchaka self-requested a review May 3, 2022 07:03
@orent
Copy link

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
Copy link
Member Author

pganssle 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.

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 fromisoformat would have a broad scope and not be configurable, and we will leave it to another library (or possibly another function) to allow further configuration.

I don't think it would be completely absurd to have a .fromrfc or fromrfc3339 method, though also RFC 3339 is so much more constrained than ISO 8601 that it's much easier to parse with something like a regex.

Doc/library/datetime.rst Outdated Show resolved Hide resolved
Doc/library/datetime.rst Show resolved Hide resolved
Doc/library/datetime.rst Outdated Show resolved Hide resolved
Doc/library/datetime.rst Show resolved Hide resolved
Doc/whatsnew/3.11.rst Show resolved Hide resolved
Modules/_datetimemodule.c Outdated Show resolved Hide resolved
Modules/_datetimemodule.c Outdated Show resolved Hide resolved
Modules/_datetimemodule.c Show resolved Hide resolved
Modules/_datetimemodule.c Outdated Show resolved Hide resolved
Lib/datetime.py Outdated Show resolved Hide resolved
Copy link

@jerub jerub left a 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.

Doc/library/datetime.rst Outdated Show resolved Hide resolved
Doc/library/datetime.rst Outdated Show resolved Hide resolved
@Fidget-Spinner Fidget-Spinner removed their request for review May 4, 2022 08:33
@orent
Copy link

orent commented May 4, 2022

I don't think it would be completely absurd to have a .fromrfc or fromrfc3339 method, though also RFC 3339 is so much more constrained than ISO 8601 that it's much easier to parse with something like a regex.

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.

@rhettinger rhettinger removed their request for review May 5, 2022 04:14
@pganssle
Copy link
Member Author

pganssle 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants