-
Notifications
You must be signed in to change notification settings - Fork 14
Consider if syntax error caused an unexpected variable instead of end #58
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
Conversation
`tIDENTIFIER` in Ruby 2.6 and `local variable or method` in Ruby 2.7 and 3.0
9529fff to
c484815
Compare
| class Blerg | ||
| Foo.call do |a | ||
| end # one | ||
| context "determines the type of syntax error to be an unmatched pipe" do |
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.
@schneems I noticed you are not really using context in the specs, would you prefer to keep these separate?
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.
My experience with contexts is that they become large and unwieldy and lose their usefulness and then when tests get moved around the indentation makes the diff larger. I'm not strictly against it though.
schneems
left a comment
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 looks great. I gave one comment on an alternate implementation. Let me know what you think.
| when /unexpected `end'/, # Ruby 2.7 and 3.0 | ||
| /unexpected end/, # Ruby 2.6 | ||
| /unexpected keyword_end/i # Ruby 2.5 | ||
| when /unexpected `end'/, # Ruby 2.7 and 3.0 |
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.
What do you think about adding a case statement above the unexpected end case. Something like:
when /unexpected .* expecting '(?<unmatched_symbol>.*)/
match = $LAST_MATCH_INFO
@unmatched_symbol = match[:unmatched_symbol].to_sym
@error_symbol = :unmatched_syntaxI'm wondering if that will allow us to simplify this a bit. My worry is the larger this case becomes the more likely accidentally firing this logic on an unrelated parse error becomes larger. By moving the "expecting " logic above this case in the detection, we can avoid trying to catch some of these seemingly unrelated other exception types.
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.
@schneems thanks, updated! Let me know what you think.
I checked and the only one that doesn't match that regexp now is unexpected `end' so I left that one there, and the rest are covered by that new regexp now 👍
Also had to use $1 instead, because for some reason $LAST_MATCH_INFO comes back as nil 🤔
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.
So I had to also leave the cases for 2.5 and 2.6, although we could use a regex there too.. what do 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.
This is great, exactly what I had in mind
Consider
tIDENTIFIERin Ruby 2.6 andlocal variable or methodin Ruby 2.7 and 3.0dead_endwas showing just the syntax error, but not the banner:With the fix: