-
-
Notifications
You must be signed in to change notification settings - Fork 36
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
Conversation
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.
Partial review
Co-authored-by: Eemeli Aro <eemeli@mozilla.com>
Co-authored-by: Addison Phillips <addison@unicode.org>
u:
options
TLDR: I am happy with namespaced attributes to represent global/universal options (non-function dependent) |
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. |
Refactored the PR to be a change/expansion of the expression-attributes design doc rather than a separate one. |
> ``` | ||
|
||
- `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: |
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.
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.
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.
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. |
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.
"Special" or "common"?
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.
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
.
exploration/expression-attributes.md
Outdated
Solve the two disparate use cases separately, | ||
so that their namespaces are not comingled. |
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.
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.
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.
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>
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.
Lots of these comments are related to the formatting and structuring of the design doc rather than to the technical arguments.
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`. |
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 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
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.
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.
exploration/expression-attributes.md
Outdated
Define at least `locale` and `dir` as options for default registry functions, | ||
with handling internal to each function implementation. |
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 you should address how id
could work here. Presumably the id
property of the resolved value could be queried?
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'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.
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 agree that we should not end up with this option. I'd probably address it by saying:
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. |
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.
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?
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.
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.
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.
Given the clarifying text about hacky identifiers added a few lines below this, is there anything actionable left in this thread?
exploration/expression-attributes.md
Outdated
Allow expression attributes to influence the formatting context, | ||
but do not directly pass them to user-defined functions. |
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.
This is too suppositional.
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. |
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'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?
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 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?
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.
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>
The core idea is to provide explicit and well-defined support for
u:id
,u:locale
, andu:dir
when used as function or markup options, and to thereby clarify that expression attributes have no runtime impact.