-
-
Notifications
You must be signed in to change notification settings - Fork 36
Add details to "Missing Selector Annotation error" section #438
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
Add details to "Missing Selector Annotation error" section #438
Conversation
* Clarify explanation of error * Add valid examples * Move one of the invalid examples to the "valid" section"
Co-authored-by: Eemeli Aro <[email protected]>
contains a _selector_ that does not have an _annotation_, | ||
or contains a _variable_ that does not directly or indirectly reference a _declaration_ with an _annotation_. | ||
contains a _selector_ that does not directly or indirectly reference | ||
an _expression_ with an _annotation_. | ||
|
||
> Example invalid messages resulting in a _Missing Selector Annotation error_: |
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.
Making the wording flow a bit better
> Example invalid messages resulting in a _Missing Selector Annotation error_: | |
> **Example**. | |
> Here are some invalid _messages_ that would result in a _Missing Selector Annotation error_: |
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 text is modeled on the existing text for the other kinds of data model errors. Perhaps a separate PR could add this change in a way that's consistent for all the error descriptions?
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 should discuss styling as a group. I'm considering creating a style guide (beyond the minimal guidance in README.md)
spec/formatting.md
Outdated
@@ -695,14 +695,23 @@ These are divided into the following categories: | |||
> when * {Value is not one} | |||
> ``` | |||
> | |||
> Example valid messages that do not result in a _Missing Selector Annotation error_: |
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 above...
> Example valid messages that do not result in a _Missing Selector Annotation error_: | |
> By contrast, here are some valid messages that do not result in a _Missing Selector Annotation error_: |
Is it that helpful to show valid messages? Shouldn't we focus on triggering 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.
I'm not sure how to clarify what "does not directly or indirectly reference an expression with an annotation" (emphasis added) means without contrasting it with a message that does indirectly reference an expression with an annotation. Do you have any suggestions for how to make that language clearer without using a valid example?
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 tend to think that providing examples of valid messages is not that helpful compared to showing invalid ones with the reason why they are invalid, or by illustrating triggering behavior with an example of how to fix it, e.g.
Here are some invalid messages that trigger a Missing Selector Annotation error:
match {$count} when * {{$count} should have an annotation like {$count :plural}} let $count = {|42|} match {$count} when * {The declaration should include an annotation like {|42| :number}}
An alternative (which is hard to do in markdown) would be to show valid vs. invalid side-by-side (as in a table) or to use HTML to create comparisons (there is HTML in my markdown but it doesn't show in a comment):
Bad example:
match {$count} when * {Missing an annotation}Good example
match {$count :plural} when * {With an annotation}
A different example would be found in one of the markdown guides, which uses tables for this effectively. See here
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 thought of another way to phrase the condition for validity that didn't require a valid example to explain -- let me know what you think.
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 haven't looked at the proposed text yet, but I will note: examples are good and even if we think of a better way of explaining something, we should be looking for opportunities to incorporate examples. My above comment might go towards some style guide activity, and, in a more general way, start to build some infrastructure to support conversion of our spec into a proper HTML document (I'll create a task for myself to capture).
* Clarify explanation of error * Add valid examples * Move one of the invalid examples to the "valid" section"
if each local _variable_ in it were replaced | ||
with the _expression_ of its binding _declaration_, | ||
repeating this process until the _expression_ | ||
contains no local _variable_s. |
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 approach is a bit problematic because option values cannot be expressions, and this is clearly including their replacement as well.
@stasm This is an example of a situation where aligning our data model closely with our syntax is making it more difficult to speak about it. If we went the other way than proposed in #436 and e.g. defined a variable expression in the data model, we could use that term here.
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.
@eemeli I see your point.
The validity property is easy to express in terms of types:
The type of a local _variable_ is the type of the _expression_ in its _declaration_.
The type of an external variable is `Unannotated`.
The type of an _expression_ is:
* `Annotated` if the _expression_ has an _annotation_.
* `Annotated` if the _operand_ of the expression is a _variable_ whose type is `Annotated`.
* `Unannotated` otherwise.
A `selector` must have type `Annotated`.
I'm not sure how this language would be received, but we are talking about a static dataflow property, whether or not we call it that :)
I also agree that it would be easier to write an unambiguous specification if the object language was the language described by the abstract syntax, rather than the concrete syntax.
I've added this to the 2023-12-11 agenda. I think we either (a) need to reject this change and let implementers do what we mean or (b) clarify how to say what we mean. |
Will reject this if no supporting comments by call time tomorrow. |
See above comment about resolution |
I found this text to be confusing: "...or contains a variable that does not directly or indirectly reference a declaration with an annotation". Read literally, this implies all variables must be annotated or indirectly reference expressions that have annotations. I tried to clarify that this only refers to variables used in a selector context.
Also, one of the "invalid" examples was actually valid, according to the "directly or indirectly" rule, because
$two
indirectly refers to{|The one| : func}
, which has an annotation. I added a "valid examples" section since I wasn't sure how else to explain the rule about indirect references.