-
Notifications
You must be signed in to change notification settings - Fork 56
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
Improve grammar after the introduction of case-in pattern matching #197
Conversation
* The value is not optional * Remove singleton 'choice' from rule
* alias 'pattern_constant_resolution' as 'scope_resolution' * replace 'pattern_constant' with just 'constant'
483652c
to
b8ef948
Compare
@@ -347,11 +347,9 @@ module.exports = grammar({ | |||
|
|||
case_match: $ => seq( | |||
'case', | |||
field('value', optional($._statement)), |
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 don't think this change is correct, since the value does appear to be optional. MRI accepts code like this:
def f(a, b)
case
when a > 1
puts 'foo'
when b == 7
puts 'bar'
else
puts 'baz'
end
end
Here is an example I found in the wild.
Compare parse.y line 3134 with line 3118.
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.
That case is covered by the case
rule. The expression case in pattern then ...
is not valid. The missing value is only allowed in combination with when
branches.
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.
Ah, sorry, that makes sense.
* array_pattern_rest => splat_parameter * hash_pattern_rest => hash_splat_parameter * hash_pattern_norest => hash_split_nil
This is more consistent with the naming of keyword_parameter
A pattern_variable is simply an identifier, no need for an additional named node.
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 went though each commit one-by-one, and they all look sensible to me.
This pull request implements some simplifications and minor fixes .
case value in ...
is no longer optionalcase_match
rule is removedconstant
andscope_expression
)*
and**
in array and hash patterns now use the same node names as the ones used in formal parameter lists (iesplat_parameter
,hash_splat_parameter
)**nil
token is now namedhash_splat_nil
and is used in patterns as well as formal parameter lists (used to be missing)pattern_pair
is renamed tokeyword_pattern
to match the naming ofkeyword_parameter
pattern_variable
was removed, it is a simpleidentifier
and there is no real need for an additional wrapper node .range
)Checklist: