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

OPTION B: Add format_unchecked functions to DateTimeFormatter via DateTimeInputUnchecked #6256

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

sffc
Copy link
Member

@sffc sffc commented Mar 8, 2025

Toward #5940

Alternative to #6255

In hindsight, this is a pretty obviously useful function. It took some experimentation until I arrived at it. I have two signatures: this one and the one in #6255. Please express an opinion on which one to move forward with.

How this approach works: it adds a single plain input type named DateTimeInputUnchecked. This type cannot be passed into format because it can't implement the required traits, so it gets its own format function with fallible behavior.

Pros and cons:

  • Pro: No weird trait bounds on this function
  • Pro: Can construct this type manually without having to implement ICU4X traits or go through ICU4X input types
  • Con: Adds a new struct type to the API that we need to bikeshed (where does it live, what is it named, what are its fields named, ...)

@sffc sffc requested a review from zbraniecki as a code owner March 8, 2025 02:23
@sffc sffc requested review from Manishearth and robertbastian and removed request for zbraniecki March 8, 2025 02:26
@sffc sffc changed the title OPTION B: Add format_unchecked functions to DateTimeFormatter OPTION B: Add format_unchecked functions to DateTimeFormatter via DateTimeInputUnchecked Mar 8, 2025
@Manishearth
Copy link
Member

Let's have the discussion on which option to pick on the other PR to keep things streamlined

@sffc
Copy link
Member Author

sffc commented Mar 9, 2025

I updated this PR to make the fields private and add a few public setter functions.

}

/// Sets the time zone UTC offset.
pub fn set_time_zone_utc_offset(&mut self, offset: UtcOffset) {
Copy link
Member Author

@sffc sffc Mar 10, 2025

Choose a reason for hiding this comment

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

Wanted to call out the setters I have in this current version of this PR:

  • set_date_fields(&mut self, input: Date<A>)
  • set_time_fields(&mut self, input: Time)

but for zone, I go field by field

  • set_time_zone_utc_offset(&mut self, offset: UtcOffset)
  • set_time_zone_id(&mut self, id: TimeZone)
  • set_time_zone_local_time(&mut self, local_time: (Date<Iso>, Time))
  • set_time_zone_variant(&mut self, zone_variant: TimeZoneVariant)

I did this because

  1. If I have type-based setters, I still have 4 setters: UtcOffset, TimeZoneInfo<Base>, TimeZoneInfo<AtTime>, and TimeZoneInfo<Full>, which I find to be more confusing
  2. These setters look sort-of like the TimeZoneInfo builder functions, so they are not without precedent
  3. What I have implemented here aligns with the needs of ZonedDateTime FFI, which is currently the only client of this function (this is a priority that caries small but nonzero weight)

CC @robertbastian, you probably have an opinion on this

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.

2 participants