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 ordinal dates #140

Merged
merged 2 commits into from
Dec 1, 2022
Merged

Conversation

movermeyer
Copy link
Collaborator

@movermeyer movermeyer commented Nov 30, 2022

What are you trying to accomplish?

Fixes #131

Add support for ordinal dates. These are the "day number" of the year. January 1st is day 1 of every year. The numbers change for leap years, meaning that you can have day 366 on leap years.

What approach did you choose and why?

  • Added a new ordinal_to_ymd method in isocalendar.c
    • Chose this file since it gave me access to _days_before_month and didn't seem worth
    • I didn't use the existing ord_to_ymd, since that is cPython's implementation that assumes an ordinal date starting at January 1 year 1, and doesn't reset each year
  • Added the code to auto-generate ordinal date test cases, and added the new exemptions for the automated invalid test case generation
  • Removed references to our "supported subset"

When I first took over maintainership and added these benchmarking scripts, there was a comment from another parser maintainer that pointed out that it wasn't a fair comparison since the other parsers could parse more of the ISO 8601 spec. At the time, I addded these warnings/disclaimers throughout the code base.

But with this PR, we now support the same subset (or more) of ISO 8601 than the other parsers do. So I've removed the disclaimers, but kept the supported subset section. It's still there (no parser supports the full spec), but doesn't need to be called out specially anymore.

  • Updated the decision tree to be simpler. Now there is no need to consider anything but ciso8601, backports.datetime_fromisoformat, or the builtin datetime.fromisoformat. 🎉

What should reviewers focus on?

You can see the rendered version of the document here.

Performance for non-ordinal timestamps is imperceptibly impacted.

The impact of these changes

We no longer have to have disclaimers everywhere. The decision tree is simple.
Once a new release is done, we can update the StackOverflow answer

We now support all the forms that other ISO 8601 parsers support
@movermeyer movermeyer merged commit 6cfa33e into master Dec 1, 2022
This was referenced Dec 5, 2022
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.

Add support for week dates and ordinal dates
2 participants