-
-
Notifications
You must be signed in to change notification settings - Fork 36
Clarifications to pattern selection #385
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
Clarifications to pattern selection #385
Conversation
As per #384 I added another commit that changes references to "decoding" keys to "resolving" them. I'm not sure if the text I added is clear enough, but am open to suggestions. (Also, maybe this should land after #382 does, since it now refers to the "Literal Resolution" section that isn't there yet.) |
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.
The current proposal seems a bit cumbersome in how it explicitly links with references the method introduction, its body, and another named section of the spec. It might be easier to drop it and include assertions about key
values in the method. The suggested change below could be prepended to all three instances, and the added lines 49-54 dropped.
The list of keys passed to this implementation-defined method | ||
does not include the catch-all key `*`. | ||
The catch-all key `*` is treated as a match for any _selector_, | ||
but with the lowest possible preference. |
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.
Wait... why are we doing this? What is wrong with letting the "implementation-defined method" decide that the *
key is the best match?
What you're requiring here is that the selector method return "no match" in order to reach *
and that the higher-level caller of the "implementation-defined method" then choose the fallback vs. allowing the selector method to make that determination. We haven't discussed this previously, so I don't understand why we are changing this text.
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 this is intended as a change to the selection method, but a clarification of what's already in the Resolve Preferences section below.
If you think that should be changed, that might be best discussed in a separate issue or PR.
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.
@aphillips Yes, as Eemeli said, it's meant to be a clarification of what's already there. The existing definitions of "Resolve Preferences", "Filter Variants", and "Sort Variants" only pass literals to "the implementation-defined method". So the method can do whatever it wants if it's passed anything that's not a literal, such as *
. Saying "The list of keys passed to this implementation-defined method does not include the catch-all key *
" is another way of saying that these definitions don't rely on undefined behavior.
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're being overly normative. In fact, the "implementation-defined method" is probably overly normative. I don't particularly like, on re-reading, the text on lines 27-29 (since my old implementation considered the selector to be a function doing the comparison and this is different from a method that "compares each to the selector")
I also don't think that the "when one selector" followed by the "when more than one selector" structure is that helpful. I would probably write this section as:
When a
message
contains amatch
statement, it determines whichexpression
matches by ordering the available keys.
In amessage
with more than one selector, the number of keys in each variant must equal the number of selectors.
Each key corresponds to the selectors in thematch
statement by position.
To determine the match, each selector is used in turn to order and filter the list of keys.
In order to select the single best variant for a given set of inputs, the full list of variants is filtered and sorted. Each variant with a key that does not match its selector is omitted from the list of variants. The remaining variants are sorted by key preference, with earlier selectors in the list of selectors having a higher priority than later ones.
The highest-sorted variant is selected.
Notice that my text is equivalent to the text here, but doesn't say how a selector is implemented or how sorting is implemented. It just says what must happen when processing a message.
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.
@aphillips Would you prefer the current proposed change to be included or dropped from this PR? Getting this merged would make it easier for you to file a subsequent PR with your proposed rewrite.
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.
On the one hand, I would prefer not to merge non-consensus text wherever possible. But I agree that the lack of other reviewers leaves little evidence about whether this should or should not go.
Let's merge this. I will file my comments as a separate issue and create a PR with them once this is merged. Sound like a plan?
Presuming a more powerful implementation which supports selection on numerical values, | ||
and provides a `:plural` function that matches keys by their exact value | ||
as well as their plural category (preferring the former, if possible), | ||
and an Enligh-language formatting context in which | ||
and an English-language formatting context in which | ||
the variable reference `$count` resolves to the number `1`, | ||
pattern selection proceeds as follows for this message: |
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.
Good typo fix.
I find this example nearly impossible to read?
I don't agree with the idea of an "English-language formatting context". It should be in terms of a locale.
I'll come back an suggest an edit when I have a few more minutes.
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.
The edit could maybe be a separate PR?
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.
Yes.
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.
Actually, on second thought... it's a typo correct. Fix the typo. We'll address the text separately.
* State up front that the catch-all key `*` is never passed to the implementation-defined key-comparison method * Clarify the relationship between selector length and key length * Clarify the type signature for `SortVariants` * Fix typo
Co-authored-by: Eemeli Aro <eemeli@gmail.com>
Change references to "the decoded value of a key" to "the resolved value of a key", and clarify that key resolution is the same as literal resolution
Co-authored-by: Eemeli Aro <eemeli@gmail.com>
…n in all instances * Also remove up-front text explaining key resolution
Co-authored-by: Addison Phillips <addisonI18N@gmail.com>
In the "Sort Variants" definition, re-specify the implementation-dependent `SortVariants` so as not to constraint whether it modifies the list in-place or returns a new list
d925bf2
to
f08b9b6
Compare
I re-requested review from @aphillips to confirm that it's OK for me to resolve the 3 open conversations, and re-requested review from @eemeli for the sorting change that I made in response to the suggestion from @aphillips ( f08b9b6 ), which seems significant enough to merit another look. |
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.
It's still good. Two stylistic nitpicks inline.
Co-authored-by: Eemeli Aro <eemeli@gmail.com>
Co-authored-by: Eemeli Aro <eemeli@gmail.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.
As noted, I'm not keen on the current structure of the text, but the changes here make the existing text consistent with the changes to the ABNF. Let's put this in and sort out my concerns separately.
In PR #385 I made the [comment](#385 (comment)) which suggested a different approach to pattern selection. This PR addresses that comment.
* Change the "pattern selection" text In PR #385 I made the [comment](#385 (comment)) which suggested a different approach to pattern selection. This PR addresses that comment. * Address comment and make terminology consistent - Make the text use semantic breaks. - Make the terminology consistent with the ABNF. In particular, the term `selector` now refers to the entire `match` statement and the term `expression` is used to refer to a specific selector expression. * Update formatting.md * Fix grammatical agreement problem. * Update spec/formatting.md Co-authored-by: Eemeli Aro <eemeli@mozilla.com> * Update spec/formatting.md Co-authored-by: Eemeli Aro <eemeli@mozilla.com> * Update spec/formatting.md Co-authored-by: Eemeli Aro <eemeli@mozilla.com> * Update spec/formatting.md Co-authored-by: Eemeli Aro <eemeli@mozilla.com> * Update spec/formatting.md Co-authored-by: Eemeli Aro <eemeli@mozilla.com> --------- Co-authored-by: Eemeli Aro <eemeli@mozilla.com>
*
is never passed to the implementation-defined key-comparison methodSortVariants