-
-
Notifications
You must be signed in to change notification settings - Fork 36
Make option values optional, defaulting to true #502
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
Did anything change in #386? I don't think there was an agreement to go ahead with this. Let's revive the discussion over there rather than jump to an 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 can't decide if I like this or not. It doesn't actually bring in boolean options, except that it does.
@@ -21,7 +21,7 @@ annotation = (function *(s option)) / reserved / private-use | |||
literal = quoted / unquoted | |||
variable = "$" name | |||
function = (":" / "+" / "-") name | |||
option = name [s] "=" [s] (literal / variable) | |||
option = name [[s] "=" [s] (literal / variable)] | |||
|
|||
; reserved keywords are always lowercase |
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.
We should reserve the keyword true
if this goes in?
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 don't think that would be required? The resolved value of a un-valued option doesn't actually show up in the syntax.
Co-authored-by: Addison Phillips <addisonI18N@gmail.com>
I do not really like APIs where missing options enable something. |
This would not be a missing option, it would be an option that is defined. The clearest existing parallel this is connecting with is HTML, which supports e.g. |
@eemeli There is a parallel that is "closer to home": boolean/flag type locale properties in a Unicode Locale Identifier canonicalize to just a key (e.g. |
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 opposed to this change. It unnecessarily complicates the language. Spelling out option=value is simple, and in fact, I think it's also clearer.
Additionally, I don't understand why this has been suddenly revived as a PR. There's #386 and the discussion in it gives reasons for not implementing this change. Those concerns have not been addressed.
@stasm's comment suggests that a design doc go first. I agree that #386 has some unaddressed bits. One small insight: MF1 has a number of these "subformat options" that are flags. There might be something useful to things like:
vs. longer/more verbose:
At the same time, imputed values that consist of the string "true" feels a little wacky. |
I don't like this. It is unclear for example if the value of such a parameter is an empty string, or boolean. Are "{$foo attr}" "{$foo attr=true}" "{$foo attr=yes}" "{$foo attr=YES}" "{$foo attr=|yes|}" "{$foo attr=||}" the same?
And there are several bugs in ICU caused by this:
So it is a parallel, and we also kind of know it's error-prone, even for those who implement ICU. |
This proposal was rejected in the 2024-01-08 teleconference. Please note that this decision does not include whether expression attributes can omit the value. That is a separate consideration. In the ldml45 release, all options must include a value. |
Closes #386
This allows for options without a value, which here default to a literal
true
. This replicates similar behaviour as used in HTML.The default value is a string, as this explicitly makes
{:foo bar}
,{:foo bar=true}
and{:foo bar=|true|}
all indistinguishable from each other, and avoids adding boolean values as a concept.In addition to function options, the utility of this syntax has been raised in discussions of expression attributes (#450, #458), which would also be able to use the same syntax.