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

Julian calendar #1351

Merged
merged 14 commits into from
Jan 7, 2022
Merged

Julian calendar #1351

merged 14 commits into from
Jan 7, 2022

Conversation

pandusonu2
Copy link
Contributor

@pandusonu2 pandusonu2 commented Dec 1, 2021

Partially fixes #1119

@pandusonu2 pandusonu2 requested a review from a team as a code owner December 1, 2021 10:40
@pandusonu2 pandusonu2 marked this pull request as draft December 1, 2021 11:00
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

good start!

components/calendar/src/julian.rs Outdated Show resolved Hide resolved
components/calendar/src/julian.rs Outdated Show resolved Hide resolved
components/calendar/src/iso.rs Outdated Show resolved Hide resolved
components/calendar/src/iso.rs Outdated Show resolved Hide resolved
components/calendar/src/julian.rs Outdated Show resolved Hide resolved
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • components/calendar/src/iso.rs is different
  • components/calendar/src/julian.rs is different
  • components/calendar/src/lib.rs is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@pandusonu2
Copy link
Contributor Author

pandusonu2 commented Dec 24, 2021

@Manishearth sorry for force push, just redid the PR, so would be better to check from start.

@pandusonu2 pandusonu2 marked this pull request as ready for review December 27, 2021 05:20
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

I think this is good! I would prefer if the offsetting code was shared, and there's some confusion around exactly what the logical types are, but we should be able to land this soon!

components/calendar/src/julian.rs Outdated Show resolved Hide resolved
components/calendar/src/julian.rs Show resolved Hide resolved
components/calendar/src/julian.rs Outdated Show resolved Hide resolved
components/calendar/src/julian.rs Outdated Show resolved Hide resolved
components/calendar/src/julian.rs Outdated Show resolved Hide resolved
components/calendar/src/julian.rs Show resolved Hide resolved
@pandusonu2
Copy link
Contributor Author

Ive added algorithms according to the book in 10585a5

But I don't really like it......
Alternative suggestion: revert 10585a5 and merge the code before that, and work on julian<->fixed integer time<->iso in another PR if we want to use that kind of conventions

components/calendar/src/iso.rs Show resolved Hide resolved
components/calendar/src/iso.rs Show resolved Hide resolved
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

Bunch of mistakes when copying over the algorithm, but the structure seems right!

components/calendar/src/julian.rs Show resolved Hide resolved
components/calendar/src/julian.rs Outdated Show resolved Hide resolved
components/calendar/src/julian.rs Outdated Show resolved Hide resolved
components/calendar/src/julian.rs Outdated Show resolved Hide resolved
components/calendar/src/julian.rs Outdated Show resolved Hide resolved
components/calendar/src/iso.rs Outdated Show resolved Hide resolved
components/calendar/src/iso.rs Outdated Show resolved Hide resolved
@pandusonu2
Copy link
Contributor Author

Ive added algorithms according to the book in 10585a5

But I don't really like it...... Alternative suggestion: revert 10585a5 and merge the code before that, and work on julian<->fixed integer time<->iso in another PR if we want to use that kind of conventions

I guess it was just a matter of rereading the algorithm multiple times till it made sense. Fine with the current changes (-.-)>

@sffc sffc removed their request for review January 6, 2022 19:22
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

Your testcases are passing now, yes?

@pandusonu2
Copy link
Contributor Author

Your testcases are passing now, yes?

Yes

@Manishearth Manishearth merged commit d87058a into unicode-org:main Jan 7, 2022
@Manishearth
Copy link
Member

Alright, thanks! Hopefully once we get the CalendarArithmetic stuff done it becomes way easier to add calendars

@pandusonu2 pandusonu2 deleted the 1119 branch January 7, 2022 09:34
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.

Implement the Coptic calendar
2 participants