Skip to content

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

Merged

Conversation

catamorphism
Copy link
Collaborator

  • 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

@catamorphism
Copy link
Collaborator Author

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.)

@catamorphism catamorphism requested a review from eemeli May 23, 2023 22:44
Copy link
Collaborator

@eemeli eemeli left a 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.

@eemeli eemeli linked an issue May 24, 2023 that may be closed by this pull request
Comment on lines +30 to +33
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.
Copy link
Member

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Member

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 a match statement, it determines which expression matches by ordering the available keys.
In a message 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 the match 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.

Copy link
Collaborator

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.

Copy link
Member

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?

Comment on lines 226 to 235
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:
Copy link
Member

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.

Copy link
Collaborator Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

Copy link
Member

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.

catamorphism and others added 7 commits May 30, 2023 18:54
  * 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
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
…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
@catamorphism catamorphism force-pushed the pattern-selection-clarifications branch from d925bf2 to f08b9b6 Compare May 31, 2023 01:55
@catamorphism catamorphism requested review from aphillips and eemeli May 31, 2023 01:58
@catamorphism
Copy link
Collaborator Author

catamorphism commented May 31, 2023

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.

Copy link
Collaborator

@eemeli eemeli left a 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.

catamorphism and others added 2 commits May 30, 2023 23:56
Co-authored-by: Eemeli Aro <eemeli@gmail.com>
Co-authored-by: Eemeli Aro <eemeli@gmail.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.

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.

@aphillips aphillips merged commit 1260c87 into unicode-org:main Jun 5, 2023
aphillips added a commit that referenced this pull request Jun 5, 2023
In PR #385 I made the [comment](#385 (comment)) which suggested a different approach to pattern selection. This PR addresses that comment.
aphillips added a commit that referenced this pull request Jun 19, 2023
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How are variant keys decoded?
3 participants