-
Notifications
You must be signed in to change notification settings - Fork 262
Add ZonedTime and use it in Combo docs #7532
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
base: main
Are you sure you want to change the base?
Conversation
| icu::properties::script::HarfbuzzScriptData#Struct | ||
| icu::properties::script::HarfbuzzScriptData::new#FnInStruct | ||
| icu::properties::script::HarfbuzzScriptDataBorrowed#Struct | ||
| icu::time::ZonedTime#Struct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix?
|
My main motivation for this PR was testing gemini-cli. I don't intend to spend more time polishing this API by adding string parsing and FFI. If that is a problem, I can mark it as |
robertbastian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main motivation for this PR was testing gemini-cli. I don't intend to spend more time polishing this API
I spent time on reviewing this PR, so I expect you to spend time addressing my comments.
^ I was asking for a reply to this. |
robertbastian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can mark it as
unstable.^ I was asking for a reply to this.
I don't want to add half-baked APIs just because you want to play around with Gemini. Either we do this correctly or we don't.
|
|
A half-baked API is better landed on main than sitting in a PR. |
|
I intend to push some code changes later when I'm back on the correct computer, but I'm still awaiting further discussion on whether If we land it, we can keep the issue open for string and FFI. |
It appears that RFC 9557 only defines suffixes as annotating a date-time, not a standalone date or time https://www.rfc-editor.org/rfc/rfc9557.html#name-abnf But, I think we allow annotations on dates? |
|
We allow annotations on dates. ISO-8601 allows times to be combined with offsets as well: https://en.wikipedia.org/wiki/ISO_8601#Time_zone_designators So we should parse the strings |
It's better to merge code than to keep PRs open. However I disagree with your statement when it comes to public APIs. I don't want to get into a situation where a non-trivial part of our API surface is unstable just because we wanted to get PRs merged. |
|
It's better to have unstable APIs because subsequent PRs to get them closer to graduation are smaller and bite-size. |
…parse a ZonedDateTime and create a ZonedTime from it
robertbastian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to have unstable APIs because subsequent PRs to get them closer to graduation are smaller and bite-size.
This PR does the trivial work of adding the ZonedTime type and its 7 GetField implementations. The real work for adding this type has always been adding a ZonedTimeFormatter on FFI, and IXDTF parsing.
The issue is not in the milestone, so nobody is going to do the actual work for this release. As I said before, I don't want to have a half-baked (more like tenth-baked) API, even unstably. So far our unstable APIs have been things that we consider done, but we want to fine-tune for one release. This is not close to done.
This also does not fix the issue like you say in the PR description.
After rereading through the issue, I also still have concerns with this type overall.
| /// "2024-10-18T15:44:00-07:00[America/Los_Angeles]", | ||
| /// Iso, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary change
| #[derive(Debug, PartialEq, Eq, Copy, Clone)] | ||
| #[allow(clippy::exhaustive_structs)] // this type is stable | ||
| #[cfg(feature = "unstable")] | ||
| pub struct ZonedTime<Z> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: this needs to be reexported in datetime::input
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine to not while this is unstable IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is sufficiently motivated (see the discussion on the issue). I'm weakly negative on adding unmotivated experimental stuff (I think that is a recipe for "too much unstable"), but this has use cases, and we've had this issue open and in various milestones for a while. So no problem with landing it as unstable from my side. API looks mostly like what I expect.
If users want this type today, they can write these 7 impls manually in userland, but I think the impls should live in the icu4x components.
The WG previously said: "OK to add this type. Cool if it makes 2.0, OK if it doesn't." It didn't make it in 2.0, so then the issue got punted to Priority Backlog. This PR is the first step to solve the issue.
We should keep the issue open.
I disagree strongly on a multiple dimensions here:
I will fix the PR description.
Please comment on the issue with any new information that should make us revisit the WG conclusion. |
Adding to this, our GetField traits are there for people to use them if they really need, BUT they're unstable and probably should remain as unstable power user APIs, so we should create stable happy paths for use cases we find. |
I have already done this in the issue since the WG decision. |
First part of #5937
Last part of #6242
This PR was written with the help of Gemini CLI with some manual tweaks, which you can see in the commit history.