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

Add some capabilities for the builtin functions #457

Merged
merged 15 commits into from
Sep 25, 2023

Conversation

ryzokuken
Copy link
Contributor

As a precursor to finalizing and standardizing the set of builtin functions, we should document the "goals" of the builtin API.

This PR aims to kick off that process by adding a simple list of capabilities that we aim to expose to users. This list is close to the set of functions exposed by the ECMA-402 API and a superset of the API documented by #420. Some notable exceptions include:

  • The list of formatters excludes relative time formatting and durations. It is my understanding that RelativeTimeFormat was never the most elegant part of the Intl API and its future use would be discouraged. Durations could be added as a follow-on, but I wasn't sure about it.

  • The list of selectors include a "gender" selector. This functionality doesn't exist in 402 and I didn't yet investigate whether it's exposed by ICU. That said, it was a recurring theme in a number of documents I read (in this repository, I think?) and I felt it might be uniquely useful within the context of MessageFormat so I decided to include it here but don't feel very strongly either way.

This is super WIP so feel free to tear it apart but I hope it's a decent launch-off point for our work.

@CLAassistant
Copy link

CLAassistant commented Aug 22, 2023

CLA assistant check
All committers have signed the CLA.

@ryzokuken
Copy link
Contributor Author

Will figure out what's the best way to deal with the CLA.

Copy link
Member

@aphillips aphillips left a comment

Choose a reason for hiding this comment

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

Thank you for starting on this!!

In the call this week we established a new bit of process. I think this PR would be better off as a design document. See here for the template. Create a new proposal using the template under /main/exploration and that'll be easier to discuss. As you can see from my comments below, trying to be "specification like" and working on what to put in the built-in registry doubles the effort.

spec/registry.md Outdated Show resolved Hide resolved
spec/registry.md Outdated Show resolved Hide resolved
spec/registry.md Outdated Show resolved Hide resolved
spec/registry.md Outdated Show resolved Hide resolved
spec/registry.md Outdated Show resolved Hide resolved
@ryzokuken
Copy link
Contributor Author

Okay moved the text to the exploration dir, made some structural changes and tried to incorporate your comments.

@aphillips how does it look?

Copy link
Collaborator

@eemeli eemeli left a comment

Choose a reason for hiding this comment

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

For some reason the prettier formatter hasn't run on this branch, and that'll need to be fixed. Otherwise, some minor issues inline.

exploration/0001-builtin-registry-capabilities.md Outdated Show resolved Hide resolved
exploration/0001-builtin-registry-capabilities.md Outdated Show resolved Hide resolved
exploration/0001-builtin-registry-capabilities.md Outdated Show resolved Hide resolved
@ryzokuken ryzokuken changed the title add some capabilities for the builtin functions Add some capabilities for the builtin functions Aug 29, 2023
@ryzokuken
Copy link
Contributor Author

ryzokuken commented Aug 29, 2023

The prettier action seems to be broken, here and elsewhere (#456). Should I file an issue about it?

@eemeli
Copy link
Collaborator

eemeli commented Aug 30, 2023

The prettier action seems to be broken, here and elsewhere (#456). Should I file an issue about it?

See #464.

Move the exploratory idea about the builtin registry from the
spec/registry.md file to the exploration directory and apply some
reviews.
@ryzokuken
Copy link
Contributor Author

I don't have the permissions to rerun this job but now it should have access to my branch 🤞

@ryzokuken
Copy link
Contributor Author

Weird, looks like prettier behaves differently but either way, this should be good now. :D

Copy link
Collaborator

@eemeli eemeli left a comment

Choose a reason for hiding this comment

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

Minor indent fix inline, but otherwise looks good to land as "proposed". So this is conditional approval presuming that #461 is accepted first; otherwise all the TBD needs to be D.

exploration/0001-builtin-registry-capabilities.md Outdated Show resolved Hide resolved
@ryzokuken
Copy link
Contributor Author

@eemeli dunno why prettier did that but thanks for the fix! I'm assuming that we're happy with #461 and that we can flesh out this document further after merging but in the meantime I'll start working on the remaining sections

@ryzokuken
Copy link
Contributor Author

Added some pretty tentative text for the other sections so that atleast all sections exist.

@ryzokuken
Copy link
Contributor Author

@eemeli @aphillips sorry for dropping the ball on this somewhat. I've now put in some improvements that I felt brought the doc closer to what you expected and applied some of Addison's comments. Please feel free to let me know if there's anything else here that needs rewording or polishing.

exploration/0001-builtin-registry-capabilities.md Outdated Show resolved Hide resolved
exploration/0001-builtin-registry-capabilities.md Outdated Show resolved Hide resolved
exploration/0001-builtin-registry-capabilities.md Outdated Show resolved Hide resolved
exploration/0001-builtin-registry-capabilities.md Outdated Show resolved Hide resolved
exploration/0001-builtin-registry-capabilities.md Outdated Show resolved Hide resolved
exploration/0001-builtin-registry-capabilities.md Outdated Show resolved Hide resolved
exploration/0001-builtin-registry-capabilities.md Outdated Show resolved Hide resolved
@eemeli eemeli mentioned this pull request Sep 7, 2023
@ryzokuken
Copy link
Contributor Author

@aphillips thanks for all the suggestions and apologies for the delay. Some of your suggestions I applied as-is, but mostly I tweaked the draft to make it closer to what I think you expected. Let me know what you think

Copy link
Member

@aphillips aphillips left a comment

Choose a reason for hiding this comment

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

Looking good. Some minor tweaks.

The use cases and requirements need to be fleshed out as we go.

@eemeli has started work on the number selection design in another PR, so maybe don't focus on that.

I think one really interesting area we need to work out are the options for numbers and for date/time values. ICU has developed "skeletons" for both of these, but JS generally has not adopted skeletons in favor of (somewhat verbosely named) bags of options. We have some consensus to use the option bags, but we should spell out the options for each and then check that it matches what both JS and ICU provide.

I think it would be interesting to consider providing an optional but defined set of options for skeletons. That is, not require an implementation to support them, but require implementations that do support them to define them using our definition.

Anyway, fix the typos and then (if the group concurs) we can merge for further work.

exploration/0001-builtin-registry-capabilities.md Outdated Show resolved Hide resolved
exploration/0001-builtin-registry-capabilities.md Outdated Show resolved Hide resolved
exploration/0001-builtin-registry-capabilities.md Outdated Show resolved Hide resolved
exploration/0001-builtin-registry-capabilities.md Outdated Show resolved Hide resolved
@ryzokuken
Copy link
Contributor Author

ryzokuken commented Sep 20, 2023

@aphillips applied your straightforward suggestions and rephrased the remaining item in "constraints". Some of the sections are far from ideal indeed but I do feel that fleshing things out as we go would be the best mode since this would just be a "proposed" draft at the moment.

@aphillips
Copy link
Member

@ryzokuken thanks!

Let's merge this and iterate on it.

@aphillips aphillips merged commit 44742a4 into unicode-org:main Sep 25, 2023
1 check passed
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.

4 participants