Skip to content

Conversation

@eemeli
Copy link
Collaborator

@eemeli eemeli commented Nov 18, 2024

This is the part of the changes originally proposed in #911 that was deemed as immediately mergeable on today's call.

Perhaps a little pre-emptively, it adopts the nomenclature to be introduced in #946.

This PR is expected to be followed up with separate PRs for the RECOMMENDED override options.

@eemeli eemeli added functions Issue pertains to the default function set fast-track Editorial change permitted to use fast-track merge rules LDML46.1 labels Nov 18, 2024
@mihnita
Copy link
Collaborator

mihnita commented Nov 18, 2024

Looking at the whole file, it looks like the way "Date/time override options" is added to :datetime and :time is inconsistent.

:datetime has it in text, but not in the list of options:

The :datetime function can use either the appropriate style options or can use a collection of field options (but not both) to control the formatted output. Date/time override options can be combined with either style options or field options.

Which is kind of hard to discover.

:time has it in the bulleted list of options

:date has it in the bulleted list of options

But it should not have it at all.
Because right now the only Date/time override option is "hour12"
Which does not apply to dates (they don't have an hour field).

@aphillips
Copy link
Member

This meets fast-track requirements and was approved in the 2024-11-18 call previously.

@aphillips aphillips merged commit 4da01dc into main Nov 19, 2024
1 check passed
@aphillips aphillips deleted the datetime-hour12 branch November 19, 2024 01:06
@mihnita
Copy link
Collaborator

mihnita commented Nov 19, 2024

was approved in the 2024-11-18 call previously

Allowing hour12 on :date is clearly wrong.
We do that just because was approved for fast-track?

The fix for that would be simple: remove it from :date


I can live with the option "hidden in text" for :datetime
It is a bit hard to discover, but not wrong.

- `long`
- `medium` (default)
- `short`
- _Date/time override options_
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this line.
The overrides right now only support hour12, which is not valid on dates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fast-track Editorial change permitted to use fast-track merge rules functions Issue pertains to the default function set

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants