Use Option to avoid failures in locale_match!

[jeremija]

This change adds postprocessing of parsed constants to determine if values are present in all locales (optional), or ensures that the types are always the same. For example, if a value was previously registerd as i64 for one locale and a String for another, it will now always be a String.

Similarly, when certain values are present in some locales but not others, the types will be wrapped in Option.

When a whole module is missing, a dummy one will be created with all consts set to None.

All of this should fix the compilation errors when trying to use locale_match! with locales that previously had diverging types.

This is most likely not a backwards compatible change, still, I'm going to double check that nothing breaks in chrono.

Closes #4.

[cecton]

It seems to be a reasonable fix with the less problem so I like the solution!

[cecton]

Small issue with the CI, you need to add the changes of the generated file to the PR.

[jeremija]

Small issue with the CI, you need to add the changes of the generated file to the PR.

🤦 just pushed an update!

[cecton]

The code looks much cleaner I love it! 👌

It would be super nice if we can get a review of @CfirTsabari on this. At least at a conceptual level. I think the idea makes a lot of sense (imo). Maybe an additional review from someone at chrono would be very useful too.

All in all it's perfect and I will merge if I don't receive more feedbacks.

[jeremija]

Thanks, I see that some tests are failing, will try to fix that ASAP (and add a few more just to make sure what I fixed doesn't break 🙂 )

Yeah it would be great if we could get more eyes on this.

For the record I tested this as a patch with chrono 0.4.30 and the compilation doesn't break, and I'm finally able to use locale_match!(locale => LC_NUMERIC::GROUPING) which was failing before!

[cecton]

For the record I tested this as a patch with chrono 0.4.30 and the compilation doesn't break, and I'm finally able to use locale_match!(locale => LC_NUMERIC::GROUPING) which was failing before!

Just for my own curiosity. Do you use that in chrono or you have a different project that (will) use pure-rust-locales?

[jeremija]

I'm working on a project where I need to format monetary and decimal values in users' locales properly and to my surprise there was no crate already available to format decimal numbers like this, so we had to implement everything from scratch. Thankfully chrono already had date formatting support through this library (that's how I learned about it) so it made sense to try to use it for our numeric use case. Hopefully one day we'll be able to open source our interger and decimal formatting crate using pure-rust-locales.

[cecton]

I think you can always call the libc to get some localized formatting. Same for the datetime formatting/translation actually. The only perk of using pure-rust-locales is because it's consistent across OS (and libc).

[cecton]

I made this crate for date and time formatting: https://crates.io/crates/libc-strftime

But I think it's pretty similar for monetary and decimal separator and such

[jeremija]

Thanks for the advice, will check out the links! Yeah we might be able to make do by using the native functions since it's a server-side app that will only ever be running on Linux, the only question is how well is it supported on dev machines which may run any other OS.

But we don't really need to support any advanced formatting, just being able to set correct number of decimal places, decimal, and thousands separators properly will most likely be enough.

Just pushed a new commit that should fix the broken tests and add a couple of more tests for locale_match! and LC_NUMERIC.

[jeremija]

Do you just run rustfmt manually after generating new src/lib.rs, or did I completely mess up the formatting with my change 😁 ?

Oh it's complaining about the source code, not about the generated code... just pushed a fix.

[jeremija]

@pitdicker 👋 according to git blame it seems you wrote most of the code in chrono which utilizes pure-rust-locales. Would you be to help us ensure this MR won't break anything in chrono if merged? 🙏

I guess we should be pretty safe anyway since it's hidden under the unstable-locales feature flag, but it doesn't hurt to have more 👀

[cecton]

Yeah don't worry there are quite some heavy tests there

https://github.com/cecton/chrono/blob/main/src/format/strftime.rs#L620

[pitdicker]

@pitdicker 👋 according to git blame it seems you wrote most of the code in chrono which utilizes pure-rust-locales. Would you be to help us ensure this MR won't break anything in chrono if merged? 🙏

I mostly moved around your code 😄. But I am happy to.

As far as I can tell the constants used by chrono are unchanged with this PR, because they were available for all locales already.
I checked the newly generated src/lib.rs and they have the same types. So that should be an easy upgrade.

Cool to have constants such as ALT_MON, FIRST_WEEKDAY and DATE_FMT available now. Maybe something we can make use of.

[pitdicker]

Tests with chrono pass with this branch.

[jeremija]

Awesome, thanks for the speedy response! 🍻

[jeremija]

@cecton do you think this could be merged now as is, or do you think we need to do further testing?

[cecton]

Yes sure! I was hoping to get more feedback but that is fine. Will do asap

[cecton]

Released 0.7.0

[jeremija]

Thanks, I really appreciate it!