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

Thin DateTime type in icu_timezone #5859

Open
Manishearth opened this issue Nov 23, 2024 · 3 comments · Fixed by #5961
Open

Thin DateTime type in icu_timezone #5859

Manishearth opened this issue Nov 23, 2024 · 3 comments · Fixed by #5961
Assignees
Labels
C-calendar Component: Calendars discuss Discuss at a future ICU4X-SC meeting S-medium Size: Less than a week (larger bug fix or enhancement)

Comments

@Manishearth
Copy link
Member

#5533 (comment)

Conclusion: Option 4 (Thin DateTime and ZonedDateTime type in icu_timezone), potentially (separate discussion) reexport DateTime from icu_datetime

Thin DateTime type in icu_timezone

  • icu_calendar becomes purely Date
  • icu_timezone has time zones and times; possibly rename to icu_time
  • IXDTF parsing can return a DateTime or ZonedDateTime, parser API lives in icu_timezone
@sffc
Copy link
Member

sffc commented Jan 16, 2025

@robertbastian pointed out that in FFI we need to do this

                .format_any_calendar(&icu_timezone::DateTime {
                    // Arc clone
                    date: date.0.clone(),
                    time: time.0,
                })

Possible solutions:

  1. Keep the status quo
  2. Make a private type in FFI that borrows things and implements GetField etc.
  3. Option 1 but put it in icu_timezone
  4. Change DateTime to be parameterized with AsRef fields

@sffc sffc reopened this Jan 16, 2025
@sffc sffc added the discuss Discuss at a future ICU4X-SC meeting label Jan 16, 2025
@Manishearth
Copy link
Member Author

@sffc I think we have another option: adding Date::wrap_calendar_in_ref() that wraps the calendar in a Ref, returning a new borrowing date. The rest of Date's fields are a cheap clone and I wouldn't be opposed to guaranteeing that the DateInner types are all Copy. (or at least adding DateInner: Copy to wrap_calendar_in_ref()

@sffc
Copy link
Member

sffc commented Jan 17, 2025

Yes that is the correct solution. We solved the ownership problem in the Date type via AsCalendar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-calendar Component: Calendars discuss Discuss at a future ICU4X-SC meeting S-medium Size: Less than a week (larger bug fix or enhancement)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants