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 calendar week to Time #6681

Merged
merged 4 commits into from
Oct 12, 2018

Conversation

straight-shoota
Copy link
Member

This PR adds two methods to Time for handling ISO calendar week:

  • Time#calendar_week returns the year and week number. The year is important because calendar weeks don't match up with calendar years and the calendar week might actually be in a different year than the regular calendar year (returned by #year).
  • Time.week_date creates a Time instance from a week date specified by year, week number and day of week.

It also adds directives %G, %g, %V which utilize these methods to Time::Format.

The algorithms for calculating the week number and back are completely annotated in code. They're based on the description on Wikipedia and the implementation in Go's stdlib but with a few improvements.

Closes #2533
Depends on #6555

Copy link
Member

@jhass jhass left a comment

Choose a reason for hiding this comment

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

I didn't review the algorithm much, but this seems more than solid enough to be accepted for a first implementation. We can always sort out edge cases as they're discovered, if any.

@RX14 RX14 added this to the 0.27.0 milestone Sep 10, 2018
@RX14
Copy link
Contributor

RX14 commented Sep 10, 2018

needs a rebase after #6555.

@straight-shoota
Copy link
Member Author

Rebased.

@straight-shoota straight-shoota force-pushed the jm/feature/time-week branch 2 times, most recently from 426d229 to beee7a6 Compare October 3, 2018 18:42
@straight-shoota straight-shoota force-pushed the jm/feature/time-week branch 2 times, most recently from 3fb2581 to f3f95ee Compare October 11, 2018 16:25
@bcardiff
Copy link
Member

@straight-shoota Sorry, could you rebase again this on master so we can merge in a more tidy fashion?

@straight-shoota
Copy link
Member Author

Sure, done. But I'm not sure what it adds, it could just be fast forwarded.

@bcardiff bcardiff merged commit c263549 into crystal-lang:master Oct 12, 2018
@bcardiff
Copy link
Member

Thanks! A rebase will not leave a text reference to the PR. Squash or merge commit will. But is better to avoid long standing branch merged and keep the repo as thin as possible.

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

Successfully merging this pull request may close these issues.

4 participants