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

Implement Duration normalization - Part 1 #20

Merged
merged 3 commits into from
Feb 14, 2024
Merged

Conversation

nekevss
Copy link
Member

@nekevss nekevss commented Feb 10, 2024

This draft PR is meant to eventually address #19

Currently, this PR is small baseline changes focused on NormativeTime records and balance methods (EDIT: it also adds CalendarDateAdd functionality for the ISO8601). The changes overall are pretty widespread across large sections of Duration that could really use more tests, so the plan is to come back to this once it can be tested a bit more or split it into a couple of PRs.

I'm going to switch this into a normal PR from a draft. After looking through it, #19 will still need to be implemented in a couple PRs, but I think it makes more sense to break the overall update down into smaller chunks

@nekevss nekevss force-pushed the duration-normalization branch from e7f92f0 to 08f7251 Compare February 10, 2024 03:40
@nekevss nekevss marked this pull request as ready for review February 10, 2024 04:08
@nekevss nekevss requested a review from jedel1043 February 10, 2024 04:08
@nekevss nekevss force-pushed the duration-normalization branch from 08f7251 to 2218d49 Compare February 10, 2024 17:51
@nekevss nekevss changed the title Implement Duration normalization Implement Duration normalization - Part 1 Feb 10, 2024
@jedel1043
Copy link
Member

I'm seeing the spec corrections make it possible to do the computations using 128bit integers. Will we transition to that representation in the future? (32 bit components + 128 bit normalized repr)

@nekevss
Copy link
Member Author

nekevss commented Feb 10, 2024

Yeah, I think it did.

To be frank, I haven't put too much thought into it as of yet. Mostly because I would really like to have a lot more of the spec implemented and some more tests in place to mitigate any edge cases or oddities that may arise off the switch.

Since f128 isn't implemented yet in Rust maybe that should be prioritized after the normalization updates.

@nekevss nekevss added this to the 0.1 Blocking milestone Feb 11, 2024
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Pretty straightforward changes, mostly because it's closely following the spec. I just have a single suggestion that doesn't block merging this.

src/components/duration/time.rs Outdated Show resolved Hide resolved
@jedel1043 jedel1043 force-pushed the duration-normalization branch from 65b4a84 to daaae7d Compare February 14, 2024 23:50
@jedel1043 jedel1043 merged commit c444d45 into main Feb 14, 2024
5 checks passed
@jedel1043 jedel1043 deleted the duration-normalization branch February 14, 2024 23:54
@jedel1043 jedel1043 added the C-enhancement New feature or request label Feb 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants