Skip to content
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

In bidi default strategy, make steps consistent with each other #969

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

catamorphism
Copy link
Collaborator

@catamorphism catamorphism commented Dec 6, 2024

I'm not sure what "If msgdir is LTR in the formatted output" meant, but this is my best guess; "let fmt be itself" was inconsistent with the other steps, which refer to "In the formatted output..." rather than setting fmt.

I think this is a normative change, since the spec was basically saying there is no formatted output that no change is made to the formatted output if step 2(iv)(a) applies.

spec/formatting.md Outdated Show resolved Hide resolved
spec/formatting.md Outdated Show resolved Hide resolved
@aphillips aphillips added formatting Agenda+ Requested for upcoming teleconference labels Dec 6, 2024
@aphillips
Copy link
Member

Adding Agenda+ so that we can discuss bidi on Monday.

and `isolate` is False,
let `fmt` be itself
append `fmt` to the formatted output.
Copy link
Member

Choose a reason for hiding this comment

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

The other steps don't do append. They talk about what fmt contains (e.g. "prefix fmt with...")

The change we should be making should actually be earlier. Something like:

  1. For each expression exp in pattern:
    i. Let fmt be the formatted string representation of the resolved value of exp to be appended to the formatted output.

@aphillips aphillips added the LDML 47 LDML 47 Release (Stable) label Dec 9, 2024
@catamorphism
Copy link
Collaborator Author

As discussed in today's meeting, I rewrote the text to specify a function from expressions to formatted strings.

@@ -928,7 +928,8 @@ Implementations MAY provide other _bidirectional isolation strategies_.

Implementations MAY supply a _bidirectional isolation strategy_ that performs no processing.

The _Default Bidi Strategy_ is defined as follows:
The _Default Bidi Strategy_ is defined as a function `B` from expressions
Copy link
Member

Choose a reason for hiding this comment

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

Linkify expressions.

This is weird? Where does B come from? Why is it a "function"? The text above this talks about "bidirectional isolation strategies". I think this needs to be different.

Perhaps:

The Default Bidi Strategy is defined as follows:

  1. Let out be an empty string to contain the formatted output of the message
  2. Let msgdir...
  3. For each expression exp in the pattern:
    1. If exp is a plain literal (text), append exp to out.
    2. If exp is an placeholder, let fmt be the formatted string representation of
      the resolved value of exp.
    3. Let dir be the directionality of fmt...
      // etc.
    4. Append fmt to out.
  4. Emit out as the formatted output of the message.

Note that we don't have a name for the non-placeholder bits of text in a pattern. I call them expressions here, but they aren't really expressions.

It would be even better if the section on formatting just above here defined how to make the string output. The "bidi strategy" then could be reference in that set of instructions ("apply the appropriate bidirectional isolation strategy to the contents of expression")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I rewrote this imperatively instead of as a function. I also did a rewrite to call the components of a pattern a "part", and distinguish between text parts and placeholder parts.

spec/formatting.md Outdated Show resolved Hide resolved
spec/formatting.html Outdated Show resolved Hide resolved
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.

This is an improvement. See inline for a few further nitpicky suggestions.

spec/formatting.md Outdated Show resolved Hide resolved
spec/formatting.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Agenda+ Requested for upcoming teleconference formatting LDML 47 LDML 47 Release (Stable) normative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants