Skip to content

Add contextual options to expression-attributes design doc #780

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

Merged
merged 11 commits into from
Jul 22, 2024
Merged

Conversation

eemeli
Copy link
Collaborator

@eemeli eemeli commented May 6, 2024

The core idea is to provide explicit and well-defined support for u:id, u:locale, and u:dir when used as function or markup options, and to thereby clarify that expression attributes have no runtime impact.

@eemeli eemeli added design Design document or issues related to design formatting Issue pertains to the formatting section of the spec labels May 6, 2024
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.

Partial review

aphillips and others added 3 commits May 6, 2024 08:24
Co-authored-by: Eemeli Aro <eemeli@mozilla.com>
Co-authored-by: Addison Phillips <addison@unicode.org>
@eemeli eemeli changed the title Add design doc for u: options Add design doc for contextual options May 6, 2024
@mihnita
Copy link
Collaborator

mihnita commented May 12, 2024

TLDR: I am happy with namespaced attributes to represent global/universal options (non-function dependent)
But that makes the attributes even less useful.
So I would suggest we drop them.

@aphillips
Copy link
Member

In the 2024-05-20 call, we agreed not to merge this PR but to have @eemeli fold this into exploration/expression-attributes.md design.

@eemeli eemeli changed the title Add design doc for contextual options Add contextual options to expression-attributes design doc May 22, 2024
@eemeli
Copy link
Collaborator Author

eemeli commented May 22, 2024

Refactored the PR to be a change/expansion of the expression-attributes design doc rather than a separate one.

@eemeli eemeli requested review from mihnita and aphillips May 22, 2024 14:34
> ```

- `dir` — An override for the LTR/RTL/auto directionality of the expression.

> Example explicitly isolating the directionality of a placeholder:
> Example explicitly isolating the directionality of a placeholder
> for a custom user-defined function:
Copy link
Member

Choose a reason for hiding this comment

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

why is a custom function wanted here? I used the implicit :string function on purpose before.

I agree that a custom function example is a good idea, just not maybe here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because the design proposed here uses u: options for contextual effects, and those require an annotation.

and must be supported by all implementations.
Common options or attributes should work the same way in different functions.

Special options or attributes should not conflict with other option names.
Copy link
Member

Choose a reason for hiding this comment

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

"Special" or "common"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Special, because that can account for implementations-specific things that may be "common" within their scope, but not universally. So e.g. @amzn:marketplace=US or amzn:marketplace=US.

Comment on lines 218 to 219
Solve the two disparate use cases separately,
so that their namespaces are not comingled.
Copy link
Member

Choose a reason for hiding this comment

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

Why are you changing the proposed design section instead of just adding the additional options? We can make changes to the proposed design when we've chosen one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because I'm proposing that we support both u: options and @ attributes?

I would like to advance this discussion towards a solution, and so I'm here proposing this as a potentially viable solution for us to agree on.

Co-authored-by: Addison Phillips <addison@unicode.org>
Co-authored-by: Tim Chevalier <tjc@igalia.com>
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.

Lots of these comments are related to the formatting and structuring of the design doc rather than to the technical arguments.

Comment on lines +284 to +288
This would mean not defining anything for default registry functions either,
effectively requiring implementation-specific options like `icu:locale`.

Other functions could use their own definitions and handling for similar options,
such as `locale` or `x:lang`.
Copy link
Member

Choose a reason for hiding this comment

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

I think spelling out the pros/cons here might be useful?

Pros

  • implementations would be free to define these options/attributes to suit local implementation needs

Cons

  • users could not count on these options to be available in each function
  • users would have to learn different requirements/restrictions
    on option use for each function and for each separate implementation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At the moment, these alternative descriptions are not formatted as pro/con lists, and I would rather keep them internally consistent. Therefore, I'd much prefer not adding such a list to just one option, and I don't want to refactor the entire PR again to include such a reformatting. We could do such a reformatting as a follow-on change.

Comment on lines 299 to 300
Define at least `locale` and `dir` as options for default registry functions,
with handling internal to each function implementation.
Copy link
Member

Choose a reason for hiding this comment

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

I think you should address how id could work here. Presumably the id property of the resolved value could be queried?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm honestly not sure how to make id sensibly work, if we're not defining it generally in the spec but only specifically for each standard registry function. That would mean not having any specific field for it in the formatting-internal resolved values, and intermingling it with the other resolved options.

I hope we do not end up with this option, and so can avoid needing to answer all the questions that it would introduce.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that we should not end up with this option. I'd probably address it by saying:

Suggested change
Define at least `locale` and `dir` as options for default registry functions,
with handling internal to each function implementation.
Define the `locale`, `dir`, and `id` options for all default registry functions
and establish a policy of including them in all future default registry functions,
with the hope that other functions and implementations will adopt this as a best practice.
Actually implementation would be internal to each function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we were to define id for default registry functions, we would also need to define what it does for them. Could you give some example for how you think that could be done without also defining id as somehow special in the spec?

Copy link
Member

Choose a reason for hiding this comment

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

Oh no, I agree with you there. These would be somehow special. If we don't reserve them outright for all functions everywhere (and instead do it by policy and hope for imitation by local functions), then people will be confused later when local functions work funny. Note that this is one argument for why expression annotations make sense to me: since they are not options, there is no danger of conflict or redefinition.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Given the clarifying text about hacky identifiers added a few lines below this, is there anything actionable left in this thread?

Comment on lines 327 to 328
Allow expression attributes to influence the formatting context,
but do not directly pass them to user-defined functions.
Copy link
Member

Choose a reason for hiding this comment

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

This is too suppositional.

Suggested change
Allow expression attributes to influence the formatting context,
but do not directly pass them to user-defined functions.
Expression attributes are stored in or modify the formatting context,
rather than passed directly to functions (including user-defined functions).
Function implementations can access the values of attributes via the formatting
context, but are not required to do so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure what you mean by "suppositional". I think what you're describing is one possible way of implementing this alternative, but it's not the only one. Should we split this option into two, one of which only defines an explicit short list of attributes with well-defined runtime effects, and the other allows for values to be stored in the expression's formatting context?

Copy link
Member

Choose a reason for hiding this comment

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

I said "suppositional" because it's hard to say what "influencing" the formatting context means?

I agree that my suggestion is one possible way of implementing this and that there could be others. However, by expressing it concretely, I hope to enable us to have a concrete conversation about which option to pursue.

I probably wouldn't go to the trouble of splitting the option up. I'd just note that my suggestion is one route and limiting the attributes to the built-in set is another?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This thread is currently marked as "outdated", as later changes to the PR made it clearer to github's diff that these lines are not changed by this PR; this was previously discussed in #458 (comment) when this text was originally added to the proposal.

Co-authored-by: Addison Phillips <addison@unicode.org>
@eemeli eemeli requested a review from aphillips June 9, 2024 17:04
@aphillips aphillips merged commit bc43db8 into main Jul 22, 2024
1 check passed
@aphillips aphillips deleted the u-options branch July 22, 2024 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Design document or issues related to design formatting Issue pertains to the formatting section of the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants