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

Add support for ISO week dates #139

Merged
merged 3 commits into from
Nov 28, 2022
Merged

Add support for ISO week dates #139

merged 3 commits into from
Nov 28, 2022

Conversation

movermeyer
Copy link
Collaborator

@movermeyer movermeyer commented Nov 26, 2022

What are you trying to accomplish?

Partly addresses #131.

What approach did you choose and why?

  • Copied the helper methods from cPython source into a new isocalendar.c

  • Added the two new branches, by porting the upstream code

  • Updated the automatic test generation code:

    • to support these cases
    • to also return the reason that the timestamp is invalid. Useful for debugging.

I also had to exempt iso_week from the "too few characters" invalid test generation for cases where it was directly followed by iso_day. If you reduce the iso_week field to 1 character, then the following iso_day will make it into YYYYWwD which gets considered to be a valid YYYYWww timestamp. We don't have this problem with non-week-date timestamps, since YYYYMM is not a valid timestamp format, and day is always two characters (so the equivalent YYYYMD cannot happen). Note that this isn't a problem change the parser, but the way the invalid test case generation code was working.

  • Updated the docs since we now support the same subset as Python itself

What should reviewers focus on?

Performance for non-week-date timestamps is imperceptibly impacted.

The impact of these changes

ciso8601 will now support the same subset of ISO 8601 as Python itself.

@movermeyer movermeyer force-pushed the movermeyer/week_dates branch 3 times, most recently from 5bbde9a to 8f79580 Compare November 26, 2022 21:25
@movermeyer movermeyer marked this pull request as ready for review November 26, 2022 21:27
@movermeyer movermeyer marked this pull request as draft November 26, 2022 21:36
@movermeyer movermeyer force-pushed the movermeyer/week_dates branch 2 times, most recently from ef0fae5 to 2b99617 Compare November 27, 2022 02:28
Copy link
Collaborator

@AlecRosenbaum AlecRosenbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good so far, but please re-request review once tests are passing.

@movermeyer
Copy link
Collaborator Author

@AlecRosenbaum Sorry about the noise. I realized that I was missing the valid case of YYYYWwwD.

@movermeyer movermeyer marked this pull request as ready for review November 28, 2022 00:52
Comment on lines +19 to +20
.. |datetime.fromisoformat| replace:: ``datetime.fromisoformat``
.. _datetime.fromisoformat: https://docs.python.org/3/library/datetime.html#datetime.datetime.fromisoformat
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a reStructured text hack to allow links on inline code blocks.

@movermeyer movermeyer merged commit 1413873 into master Nov 28, 2022
@movermeyer movermeyer mentioned this pull request Dec 7, 2022
@movermeyer movermeyer deleted the movermeyer/week_dates branch December 21, 2022 18:51
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.

2 participants