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

(Design) Extending functions/options/namespacing #475

Merged
merged 17 commits into from
Nov 13, 2023

Conversation

aphillips
Copy link
Member

@aphillips aphillips commented Sep 13, 2023

Here is the design document for extending functions and options and the namespacing thereof.

This describes the namespacing elements and some of the normative text needed, along with early ABNF changes.

Note that I extended the reach of namespaces to:

  • functions
  • options
  • expression attributes

@eemeli

This comment was marked as outdated.

@aphillips

This comment was marked as outdated.

@eemeli

This comment was marked as outdated.

Copy link
Collaborator

@stasm stasm left a comment

Choose a reason for hiding this comment

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

A quick first pass of my review, but this is looking very much in line with what I had imagined. Thanks for writing it!

It would be great to also include a list of changes to the spec that we'll need to make as a result of this design, so that we can track them and make sure the PR that follows the design is comprehensive. For example:

  • From the use-case about tooling, I gather we'll want to design changes to the data model, too?
  • The function resolution section of formatting.md. How are implementations supposed to resolve namespaced functions?
  • The error handling section of formatting.md. What happens when a namespace is not known to the runtime?
  • Are any changes to the registry required? (It doesn't seem so.)

exploration/0475-overriding-extending-namespacing.md Outdated Show resolved Hide resolved

_What prior decisions and existing conditions limit the possible design?_

- A syntactical prefix must not collide with `nmtoken`, to avoid parsing ambiguities with unquoted literals...
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if nmtoken limits us here. Function names are recognized by the : prefix and function options can only appear in very specific positions.

Instead, it might make sense to add a constraint about : being already used in the syntax. Namespace syntax can leverage this fact (like this proposal suggests, which I like), or attempt to avoid it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added notes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I think I understand why we you included this constraint; sorry I missed this earlier. We're effectively saying here that we can't get rid of the : in front of the annotation, even if it's namespaced.

Sounds like something worth documenting in #478.

exploration/0475-overriding-extending-namespacing.md Outdated Show resolved Hide resolved
> with many different keyboards need to be able to enter the prefix.

The namespace prefix is part of the `name` production.
The prefix is limited to eight characters in length and MUST be at least two characters
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the purpose of the 8 character limit?

Copy link
Member Author

Choose a reason for hiding this comment

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

brevity. I meant to include something about the limit, but we should probably make it unlimited.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed limit and changed wording.

exploration/0475-overriding-extending-namespacing.md Outdated Show resolved Hide resolved
exploration/0475-overriding-extending-namespacing.md Outdated Show resolved Hide resolved
exploration/0475-overriding-extending-namespacing.md Outdated Show resolved Hide resolved
- Developers must be able to write functions that do not later collide with items in the default registry.
- Developers must be able to write function add-ons that do not later collide with items in the default registry.
- Users should be able to tell visually when an add-on feature has been used vs. a built-in
- Users should be able to resolve conflicts between add-on packages that use the same
Copy link
Collaborator

Choose a reason for hiding this comment

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

All of these requirements combined mean that developer are in control of choosing the namespace name. I think that's by design and desirable, but we should address in the doc the negative impact this can have on portability of messages.

Comment on lines 129 to 131
If an implementation supports user-installed formatters, selectors, function options,
or expression annotations, it must also support providing "namespace" prefixes for
each installed set of functionality.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do I understand right that with this approach, an implementation could choose to use pretty much any pattern for establishing namespacing, and have it internally work the same as the proposed ns:func?

I'm not really seeing the benefit of explicitly defining a separator sigil, as opposed to providing functionality that would let a name prefix like "ns:" associate itself with a URL defining its functionality.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this means that if implementations want to go beyond std:, they need to allow users to namespace addons. The pattern for doing so is ns:func, and that wouldn't change.

exploration/0475-overriding-extending-namespacing.md Outdated Show resolved Hide resolved
exploration/0475-overriding-extending-namespacing.md Outdated Show resolved Hide resolved
exploration/0475-overriding-extending-namespacing.md Outdated Show resolved Hide resolved
Comment on lines 90 to 99
- Developers want to import 3rd party formatting packages and use the package's
features from within messages.

- Users want to import two or more formatting packages
and these might have the same-named functions.
For example, there might be both an HTML `p` and TTS `p`
function.

- Users want to control how extensions are referenced in their messages.
For example, they might wish to make a long namespace name shorter.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've been thinking about these 3 use-cases, and I think there's a portability risk that we should call out.

I see two groups of users who could be in charge of controlling the namespace name:

  • (a) Authors of custom functions bundled in "packages"; this is similar to how C/C++ or Java handle naming.
  • (b) Developers who import those packages into their MFv2 implementation of choice; this is in line with how modern languages handle imports and namespaces, via some sort of import ... as mechanism.

The current solution proposed by this PR is to go with (b), and allow developers to adjust the namespace if necessary. The risk this brings is that these custom monikers chosen by developers are then hardcoded inside messages, hurting the portability of messages.

Example: If I choose myFoo as a namespace, this naming choice propagates down to all messages (myFoo:number etc.). These messages then require edits when they're migrated to another project, when codebases merge, or even when I change my mind and rename the namespace to myBar.

We can attempt to avoid or mitigate this risk (or accept it).

  • Avoid: Go with (a), i.e. authors choose and control the namespace name. Since this is still likely to produce conflicts, a naming scheme similar to Java's fully qualified names would be recommended. Unfortunately, it means that namespaced names become domain.name.like long.

  • Mitigate: Go with (b), i.e. developers importing packages can give them custom names. Embrace the fact that those custom names are then hardcoded in mesasges and make it easy for tooling to work with them and modify them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with this callout. The challenge with (a) is that then you need a fairly sizeable namespace that can be made unique by function authors, e.g. it suggests reverse domain naming.

There is a third path, which is basically (b) plus a registry (think: media types). I am not wild about creating a registry, but that would help with the portability and toe-stepping problem (while allowing users to control the prefix for their own space).

Since there is no library ecosystem currently, we can also create the mechanism but not flesh it out for 2.0. That is, we can reserve the syntax but come back later to define its usage. In some ways that's like @eemeli's proposal (it's just a convention), except that it is formally reserved (if you name a function with an interior :, it might not work later)

Each function, option, or option value extension needs to work as seamlessly as possible
with other add-ons and with the built-in functionality.

To that end, we need to define how externally-authored functions appear in a _message_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this paragraph should come immediately after the heading. As it is, the first sentence after _What is this proposal trying to achieve?_ doesn't answer the question.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Moved to start and slightly reworded for context.

exploration/0475-overriding-extending-namespacing.md Outdated Show resolved Hide resolved
exploration/0475-overriding-extending-namespacing.md Outdated Show resolved Hide resolved
> ```
>
> Everything altogether all at once. This probably does not work
> correctly, since `std:datetime` may not understand `icu:skeleton`:
Copy link
Collaborator

Choose a reason for hiding this comment

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

So can the example use icu:datetime instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good callout. I made a different change: I made this example use a built-in option and then did icu:skeleton for options after.

@aphillips
Copy link
Member Author

We agreed to merge this today (2023-11-06) as proposed, but asking for one more review before I do that.

@@ -0,0 +1,337 @@
# Design Proposal Template
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Design Proposal Template
# Extending functions and their options

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or more simply:

Suggested change
# Design Proposal Template
# Namespacing

Copy link
Member Author

Choose a reason for hiding this comment

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

I went with:

Design Proposal: Namespaces for Extending Functions, Options, etc.

exploration/overriding-extending-namespacing.md Outdated Show resolved Hide resolved
exploration/overriding-extending-namespacing.md Outdated Show resolved Hide resolved
Comment on lines +84 to +85
- Developers need to add options to the base functions to suit local needs.
For example, ICU's skeletons as part of the `:datetime` function
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Developers need to add options to the base functions to suit local needs.
For example, ICU's skeletons as part of the `:datetime` function
- Developers need to add options to the base functions to suit local needs:
for example, ICU's skeletons as part of the `:datetime` function.

@aphillips aphillips changed the title Design document for extending functions/options/namespacing (Design) Extending functions/options/namespacing Nov 12, 2023
@aphillips aphillips merged commit 6ede41b into main Nov 13, 2023
2 checks passed
@aphillips aphillips deleted the aphillips-extending-formatting branch November 13, 2023 17:50
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.

5 participants