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

Fix fallback value definition and use #903

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

Fix fallback value definition and use #903

wants to merge 14 commits into from

Conversation

eemeli
Copy link
Collaborator

@eemeli eemeli commented Oct 10, 2024

This is a continuation for the changes of #728, defining a fallback value explicitly as a resolved value, and rephrasing the parts of the spec that refer to resolution failures to instead test for whether a resolved value is a fallback value.

@eemeli eemeli added formatting LDML46.1 MF2.0 Draft Candidate labels Oct 10, 2024
spec/formatting.md Outdated Show resolved Hide resolved
spec/formatting.md Outdated Show resolved Hide resolved
spec/formatting.md Outdated Show resolved Hide resolved
spec/formatting.md Show resolved Hide resolved
spec/formatting.md Outdated Show resolved Hide resolved
spec/formatting.md Outdated Show resolved Hide resolved
spec/formatting.md Outdated Show resolved Hide resolved
spec/formatting.md Outdated Show resolved Hide resolved
Co-authored-by: Addison Phillips <addison@unicode.org>
spec/formatting.md Outdated Show resolved Hide resolved
spec/formatting.md Outdated Show resolved Hide resolved
spec/formatting.md Outdated Show resolved Hide resolved
spec/formatting.md Outdated Show resolved Hide resolved
Co-authored-by: Tim Chevalier <tjc@igalia.com>
spec/formatting.md Outdated Show resolved Hide resolved
spec/formatting.md Outdated Show resolved Hide resolved
spec/formatting.md Outdated Show resolved Hide resolved
spec/formatting.md Outdated Show resolved Hide resolved
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
spec/formatting.md Outdated Show resolved Hide resolved
spec/formatting.md Outdated Show resolved Hide resolved
spec/formatting.md Outdated Show resolved Hide resolved
spec/formatting.md Outdated Show resolved Hide resolved
but it always resolves to some mapping of string identifiers to values.
The result of _option resolution_ MUST be a (possibly empty) mapping
of string identifiers to values;
that is, errors MAY be emitted, but such errors MUST NOT be fatal.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the first mention I've noticed in the spec of a non-fatal error, i.e. a warning. If Bad Option is sometimes a fatal error and sometimes a warning, it seems like the spec needs some more language to clarify what implementations should do with it. Or if it's meant to always be a warning, maybe https://github.com/unicode-org/message-format-wg/blob/main/spec/errors.md should say something about that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

On second thought: from the perspective of an implementation can only signal errors or return usable output, but not both, I'm reading this as saying that the implementation can't signal a Bad Option error. Which is fine, since it's a "MAY".

Co-authored-by: Tim Chevalier <tjc@igalia.com>
aphillips added a commit that referenced this pull request Oct 28, 2024
This addresses concerns raised in #903 

- define `option resolution` as a term
- require that option order is not significant

I was tempted to define the term `resolved options`, but held back.
aphillips added a commit that referenced this pull request Oct 28, 2024
This addresses concerns raised in #903 

- define `option resolution` as a term
- require that option order is not significant

I was tempted to define the term `resolved options`, but held back.
@eemeli
Copy link
Collaborator Author

eemeli commented Nov 12, 2024

Updated, as per today's discussions. The fallback variable name is now the last one, and it may be a local variable name.

@@ -259,6 +259,12 @@ The resolution of a _variable_ fails if no value is identified for its _name_.
If this happens, an _Unresolved Variable_ error is emitted
and a _fallback value_ is used as the _resolved value_ of the _variable_.
If the _resolved value_ identified for the _variable_ _name_ is a _fallback value_,
a _fallback value_ is used as the _resolved value_ of the _variable_.
Copy link
Member

@macchiati macchiati Nov 12, 2024

Choose a reason for hiding this comment

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

A general issue: The resolved value is logically {datatype, optionMap}. So whenever we have a statement like this, that implies that a fallback value is a resolved value. That means we must say what the optionMap is: I presume an empty map, but we need to be clear.

I think for this case, we can probably put this in one place, the definition of fallback value. We should check other instances of 'resolved value' for completeness.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. I've updated the text to make the references more explicit. PTAL?

Copy link
Member

Choose a reason for hiding this comment

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

Excellent fixes to all the fallback value text, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatting LDML46.1 MF2.0 Draft Candidate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants