-
-
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
Unify input and local declarations in data model #799
Conversation
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 think I've commented on this before, but the implication of this change is that either:
- the parser has to check for all duplicate declaration errors
- duplicate declaration error checking has to be split, with a special case in the parser for
.local $x = {$x :func}
, and then the more general check in a subsequent error-checking pass.
IMO it's useful for .local $x = {$x :func}
to be representable in the data model, so that all duplicate declarations can be checked in a single pass that's separate from the parser.
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 PR does seem to introduce an unnecessary complexity (and thus burden) on implementations, as @catamorphism. As I understand it, this PR would require that an implementation's parser would have to complect together the separate concerns of parsing and duplicate declaration.
I like the attempt at simplifying the spec, but it currently doesn't seem to me to an improvement b/c it undoes an important distinction. Although, this discussion is useful and perhaps can be converted into important documentation explaining why these constructs have to remain separate. WDYT?
FWIW, for the record, even though the PR description says that it represents the uncritiqued parts of the discussion in #718, I see concerns on this topic in that discussion that didn't get resolved.
If the `value` has a `VariableRef` `arg` with the same `name` | ||
as the `Declaration`, | ||
it represents an _input-declaration_. | ||
Otherwise, it represents a _local-declaration_. |
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'm not fond of the blanket value: Expression
, which requires this wording here. I'd much prefer if the data model didn't need these additional validity constraints. I think the current data model achieves this to a larger extent.
I wouldn't want anyone to think that it's OK to have .input {:func}
or .input {|1|}
-- which I think the proposed change does suggest.
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 change is in fact removing a MUST validity constraint on InputDeclaration
, in addition to removing a similar implicit constraint on the LocalDeclaration
that it must not contain in its value
a VariableExpression
which uses the same name
as used at the top level of the declaration.
Could you explain how this proposed language suggests that something like .input {:func}
is ok? As it explicitly requires the expression to have a VariableRef
argument with a matching name
, isn't that the same set of requirements that are expressed in the syntax?
spec/data-model/README.md
Outdated
annotation?: FunctionAnnotation | UnsupportedAnnotation; | ||
attributes: Attributes; | ||
} | ||
type Expression = OperandExpression | AnnotationExpression; |
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 is an interesting suggestion worth pursuing in a separate PR.
I don't think these should be unified, they look the same, but are different concepts. For example a translator can define as many local variables as they want. Similar to programming languages:
TLDR: not unify, they look superficially the same, but are different concepts. |
Responding to @echeran:
With this change, the Responding to @mihnita:
As discussed during the 2024-07-15 call, there are situations in which it may be valid for a translator (or their tooling) to add a I can provide a real-world example with this Firefox message, which in MF2 is effectively
in the source locale (en-US), but its Japanese translation depends on the OS, requiring the use of a custom
Note how the introduction of the MF2 allows for this, while also allowing for a specific user to choose to restrict changes such as the above. It's also good to note that similar restrictions on the input may also be imposed by a
As in the example above using In other words, the more appropriate "MUST NOT" to impose on translators (e.g. via tooling) is changes to expressions where the operand is externally provided. Say, if the original was instead
then it would not be valid to change the expression to That is a restriction which applies to all |
Discussed in 2024-09-10 call. Moving to post-46 but pre-2.0 |
This change was rejected in the 2024-10-14 call. |
Closes #786
This change is a part of what was discussed in #718, i.e. the part that didn't get any critique in the issue.
In the syntax, keeping
.input
and.local
as separate operations makes really good sense from a readability point of view. In the data model, as those concerns are not present, there is no reason to keep the separatetype: 'input'
andtype: 'local'
blocks. Both will almost certainly be processed the same way, and the input/local-ness can be inferred when necessary byThis also drops the need for a
VariableExpression
, which simplifies the TS & JSON Schema definitions quite a bit.