Skip to content

Update ixdtf to 0.4.0#219

Merged
jedel1043 merged 2 commits intoboa-dev:mainfrom
Manishearth:ixdtfup
Feb 28, 2025
Merged

Update ixdtf to 0.4.0#219
jedel1043 merged 2 commits intoboa-dev:mainfrom
Manishearth:ixdtfup

Conversation

@Manishearth
Copy link
Copy Markdown
Contributor

I can't figure out how to construct a fraction, do we need to add APIs upstream?

It may also be worth it to add a direct .to_milliseconds_and_microseconds() API to Fraction.

@Manishearth
Copy link
Copy Markdown
Contributor Author

I'm happy to do a quick patch release with the necessary APIs to ixdtf if @nekevss can propose what they should look like

@jedel1043 jedel1043 added the C-internal Internal library improvements label Feb 27, 2025
@nekevss
Copy link
Copy Markdown
Member

nekevss commented Feb 27, 2025

😕 hmmmmm

So some background: the formatting logic I was potentially going to see if there was any interest in upstreaming into ixdtf, but I hadn't exactly gotten to that point of proposing it. That's the reason the the FormattableX structs exist with some internal ixdtf records.

So baseline a baseline option: we could just remake the functionality in temporal_rs, and live with the differences.

With that being said, if there were any constructor added. It may be a good idea to at minimum upstream the Precision option into ixdtf, then fraction could be something like:

impl Fraction {
    fn from_value_with_precision(value: u64, precision: Option<Precision>)  -> Option<Fraction> {
        match precision {
            None if value == 0 => None,
            None => todo!(), // Derive digit precision from the length of value 
            Some(Precision::Minute) => None,
            Some(Precision::Digit(digit)) => Fraction { digit, value },
        }
    } 
}

Although in this scenario, Precision::Digit would probably need to be a NonZeroU8.

@nekevss nekevss requested a review from jedel1043 February 28, 2025 02:47
Copy link
Copy Markdown
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.

Looks great!

@jedel1043
Copy link
Copy Markdown
Member

Will rebase since this also bumps ICU4X, just to see that it merges cleanly.

@jedel1043 jedel1043 merged commit d49285f into boa-dev:main Feb 28, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-internal Internal library improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants