-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
Update selection-declaration design doc based on mtg / issue discussion #867
Merged
aphillips
merged 1 commit into
unicode-org:main
from
echeran:selection-declaration-edits
Aug 26, 2024
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 might be clearer if we show separate annotation?
... or is that not your intention?
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 find this to be clear.
It is in fact what we have today.
And depending on the mental model of the reader, it is unclear if the
{$person}
inside the message is the:gender
one or the:personName
one.It is in fact what started the issue, that I can do stuff like this:
For clarity I can imagine two options:
.local
, which gives us a different name:OR
.match
cannot have any function / parametersCase 2 would not be useful in this case, with gender, but would be when we want consistency, in plural
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 already decided for immutability in
.local
and.input
WDYT about thinking of .match also as some kind of immutable?
These would be the rules:
if
.match on $foo
has any function / attributes / options on itif there was a
.local $foo = {...}
or.input {$foo ...}
beforeERROR
else
$foo is "bound" to the function / attributes / options in
.match
for the rest of the message// This gives us consistency between the
:number
selection / formattingelse
if there was a
.local $foo = {...}
or.input {$foo ...}
before, with a function on ituse that exact definition ("binding") to do the selection // What we do today
else
ERROR, because the items on which we select must always have a function
// So that we know what we select on, what keys might be valid, etc.
// This is something we agreed a long time ago, useful for both linting, L10N tools, and human translators
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.
Exercising the idea above:
=> ERROR.
$person is "bound" to two different types / transforms / functions
Or
.match
tries to change an immutable.input
, if you want to think of it in terms of immutability=> CORRECT
We have two "variables",
personName
andperson
, no conflicts, everything is clearCORRECT
CORRECT
ERROR
Even if the function name is the same, it is still confusing.
Is
.match {$count :number}
inheriting the options from.input
or not?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.
My intention is to highlight that selection and formatting may require different functions. My example accomplishes that straightforwardly without getting into tangential questions about function composition, tradeoffs of concision vs. simplicity, etc.
I was initially confused by your example, even though it's more concise, because it wasn't making sense to me. If
.input
is binding the value of the expression{$person :personName}
back to$person
, then we no longer have the full person object accessible to provide to the:gender
function in the subsequent line performing selection:.match {$person :gender}
. Am I reading that right? If so, that of course highlights the outstanding question of how we should define function composition, but that's a separate topic.Also, the example might engender the reader to want to declare a
.local person-name {$person :personName}
for all the repetition in the patterns, but then if they do, they would realize that such refactoring is independent of selection. So the idea of requiring selection to take place only on variables (the design doc alternative named "Match on variables instead of expressions"), which in turn would require a.local
declaration for the selector expression{$person :gender}
, would not buy us anything in terms of consistency within the message because there was never a linkage between the selector expression function and pattern placeholder formatting 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.
@mihnita Having read your logic and explanation, it makes sense and I personally wouldn't mind this sort of conditional mutability, but as I've stated elsewhere I just see this as overly complicated and most developers are not going to invest the time to fully grasp what will happen under these different conditions, which ultimately just trades slightly increasing flexibility and conciseness for much more confusion and errors.
I think something more rigid like "selection never mutates" is incredibly clear, and barely harms the experience, if at all. You can repeat your expressions or use an input/ local.
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.
Note that this discussion is about text that @echeran is helpfully suggesting in the design document... a document that, I'll point out, talks about all of the issues on this thread and whose point is to illustrate choices we might make to resolve the problems it raises. @echeran has answered my question, so I'm happy to merge this so that we can have the actual discussion 😄