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

Should we re-export and/or move input types to the formatting crates #6069

Open
sffc opened this issue Feb 5, 2025 · 3 comments
Open

Should we re-export and/or move input types to the formatting crates #6069

sffc opened this issue Feb 5, 2025 · 3 comments
Labels
C-meta Component: Relating to ICU4X as a whole

Comments

@sffc
Copy link
Member

sffc commented Feb 5, 2025

For example, move FixedDecimal aka Decimal to the icu_decimal crate. Could also apply to datetime.

@sffc sffc added C-meta Component: Relating to ICU4X as a whole discuss Discuss at a future ICU4X-SC meeting labels Feb 5, 2025
@sffc sffc added this to the ICU4X 2.0 Stretch ⟨P2⟩ milestone Feb 5, 2025
@Manishearth
Copy link
Member

  • @sffc icu::decimal and icu::datetime - do you have feedback on whether we should reexport those types? Currently, we have to import types from two different crates: there is the crate for the input type to the formatter, and the crate for the formatter.
  • @hsivonen So the question is about re-exporting?
  • @sffc The question is about reexporting the types. For example, we could reexport the fixed_decimal type in the icu::decimal
    • For example: icu::decimal::input::Decimal or icu::datetime::input::Date, or maybe not with the input module. These would be re-exports.
  • @hsivonen I don't have proper experience of this problem space to the extent that I have a good take on this. My initial reaction is that re-exports make it harder to read docs. I understand the issue that if the caller of the formatting crate needs to declare a dependency on two different crates, it has more moving parts and may break the build more easily. But I don't know how to properly assess the downside of reading docs.
  • @robertbastian I think for docs, there are two ways to show reexports: inline and no_inline. We would want to show them as no_inline, so pub use <calendar_crate>::Date. We don't publish the metacrate, so we don't need to worry about that.
  • @hsivonen The metacrate makes this discussion easy because
  • @sffc Something I worked on recently can show in the docs description the link to the original definition of the type in the place where it is reexported.
  • @robertbastian I find this even more confusing.
  • @hsivonen I don't have enough experience to have a strong opinion here.

@Manishearth
Copy link
Member

First: discuss #6104: notes in #6104 (comment)

Back to #6069

  • @Manishearth Do we expect people to use the fixed_decimal type outside of formatting?
  • @robertbastian I don't think so? Somewhere I've used the Display impl for FixedDecimal.
  • @Manishearth That sounds like a fallback from not being able to format.
  • @robertbastian A user might want that, and say, "I just want a formatter without i18n formatting".
  • @Manishearth Is it our job to... do we think there is value for us or the ecosystem maintaining a crate for that purpose?
  • @robertbastian Probably not. Are you thinking about completely inlining it? In the past, we have evolved teh semver of fixed_decimal independently of ICU4X semver.
  • @Manishearth Do we want to provide this artifact independent of i18n functionality?
  • @robertbastian Or, do we want to provide this artifact independent of the data?
  • @Manishearth I don't think people will benefit
  • @sffc First off, yes, I think it's useful. It's the only crate in the Rust ecosystem that works on this data model. It's the only Rust crate that represents base 10 via a digits list. Other crates represent decimal numbers using binary. For us, to combine the integer and fractional portions of a decimal number is a cheap operation. There's no funny business to support rounding modes as there is for binary numbers because we directly support decimal numbers. Secondly, fixed_decimal is a dependency for both icu::plural_rules and icu::decimal, but these crates should not be dependent on each other.
  • @Manishearth The fact that this create is a dependency on plural_rules is reason enough to keep this crate be separate.
  • @sffc I think we should do it. I think we should re-export under a specific module called input.
  • @robertbastian DecimalFormatter has one type and lots of options.
  • @sffc Should we reexport everything? Or just what the crate needs?
  • @robertbastian I don't think should model any policy around what we need to do in icu::decimal.
  • @sffc Let's talk about icu::decimal now in isolation. Thereafter, we can talk about about icu::datetime.
  • @robertbastian pub use fixed_decimal as input seems fine? With #[doc(no_inline)]
  • @Manishearth That would still have the doc linking problems
  • @sffc The fact that we take a fixed_decimal as an input already implies that we have a semver. And we should only export the ones that are needed.

Conclusion on #6069 for Decimal:

  1. Re-export Decimal as icu::decimal::input::Decimal
  2. Re-export the transitively reachable types from Decimal that are relevant for DecimalFormatter (example: SignDisplay but not CompactDecimal)
  3. Use a similar doc-inlining strategy as Reexport preferences enums from component::preferences #6104
  4. Update all of the docs in icu_decimal to import Decimal from its new re-export

LGTM: @robertbastian @Manishearth @sffc @echeran

Discussion on icu_datetime:

  • @sffc Should we export the calendars?
  • @robertbastian It's a lot to maintain if we don't just re-export the whole cal module
  • @Manishearth I'd like to avoid exporting scaffold
  • @sffc Type inference works well enough that we can probably not export cal
  • @sffc Should we export the ixdtf feature?
  • @robertbastian Half the people will want to construct these types from strings
  • @Manishearth Yeah, sub-crate features are annoying
  • @robertbastian - So what about ZonedDateTimeParser? We could nuke the type and have parse methods on ZonedDateTime that accept IanaParser and UtcOffsetCalculator. This would actually be more efficient as both of them are not always needed
  • @Manishearth Maybe the rule of thumb is that we don't include things that load data
  • @robertbastian We no longer need ZoneVariant with ZoneVariant constructors from isdst #6113.
  • @sffc Are we keeping the try_from_str data-dependent things on the input types?
  • @robertbastian All these functions require passing in explicit data.
  • @Manishearth If we start moving everything into datetime::input, we lose all the design work we've done in the other crates
  • @sffc Looking through the docs of icu::datetime, the input types are the only ones we typically need in the examples. Sometimes we import Gregorian explicitly.
  • @Manishearth Should we re-export Gregorian? Seems more common of a use case than Japanese for instance.
  • @robertbastian No, if we think it's that common, we should export GregorianDateTimeFormatter like we do in FFI.

Conclusion:

  1. Re-export Date, Time, DateTime, ZonedDateTime, TimeZone (aka TimeZoneBcp47Id), and UtcOffset from the icu::datetime::input module
  2. Add an ixdtf feature to icu_datetime
  3. Consider adding a GregorianDateTimeFormatter alias in the future

LGTM: @sffc @Manishearth @robertbastian

Brief re-visit of #6069 for Decimal:

  • @sffc Given our decision on icu::datetime::input containing only the actual input types, do we want to revisit our decision to put SignDisplay, RoundingMode, etc., in icu::decimal::input versus somewhere else like icu::decimal::options or icu::decimal::input::options?
  • @robertbastian It's weird because it's options on the input, not options on the formatter. I don't have a strong opinion.
  • @Manishearth I think it's fine to have them flat; it's not like DateTime where there are a lot of different input types that go in. And there are a couple more things. I don't see the input module as a place to look for what you can pass in for decimal, maybe only for datetime.
  • @sffc Are we exporting the errors?
  • @Manishearth I think not. It's more of a fixed_decimal task than a formatting task.
  • @sffc Ok, so I think the input types end up being structs, and the options end up being enums, so we get that distinction for free in rustdoc.
  • @Manishearth I think we shouldn't rely on that distinction
  • @sffc I think we have an agreement even for different reasons

Conclusion: keep the previous conclusion.

@sffc sffc removed the discuss Discuss at a future ICU4X-SC meeting label Feb 14, 2025
robertbastian added a commit that referenced this issue Feb 14, 2025
I've not prettified docs, because they'll be touched again by #6069 and
#5999.

Part of #6119
@sffc
Copy link
Member Author

sffc commented Feb 14, 2025

Note: I'm ok exporting both TimeZone and TimeZoneInfo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-meta Component: Relating to ICU4X as a whole
Projects
None yet
Development

No branches or pull requests

2 participants