Skip to content

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.

Sign up for GitHub

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

Merged
schneems merged 3 commits into from
Feb 25, 2021
Merged

Consider if syntax error caused an unexpected variable instead of end #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c484815
Consider if syntax error caused an unexpected variable instead of end
mauro-oto Feb 13, 2021
b00ea76
Use regexp for handling similar scenarios across Ruby 2.5 - 3.0
mauro-oto Feb 25, 2021
66ac998
Handle Ruby 2.5 and 2.6 separately
mauro-oto Feb 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
## HEAD (unreleased)

- Consider if syntax error caused an unexpected variable instead of end (https://github.com/zombocom/dead_end/pull/58)

## 1.1.5

- Parse error once and not twice if there's more than one available (https://github.com/zombocom/dead_end/pull/57)

## 1.1.4
Expand Down
14 changes: 7 additions & 7 deletions lib/dead_end/who_dis_syntax_error.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,14 +51,14 @@ def on_parse_error(msg)
when /unexpected end-of-input/
@error_symbol = :missing_end
when /expecting end-of-input/
@error_symbol = :unmatched_syntax
@unmatched_symbol = :end
when /unexpected `end'/, # Ruby 2.7 and 3.0
/unexpected end/, # Ruby 2.6
/unexpected keyword_end/i # Ruby 2.5

match = @error.match(/expecting '(?<unmatched_symbol>.*)'/)
@unmatched_symbol = match[:unmatched_symbol].to_sym if match
@error_symbol = :unmatched_syntax
when /unexpected .* expecting '(?<unmatched_symbol>.*)'/
@unmatched_symbol = $1.to_sym if $1
@error_symbol = :unmatched_syntax
when /unexpected `end'/, # Ruby 2.7 and 3.0
Copy link
Collaborator

@schneems schneems Feb 22, 2021

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_syntax

I'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.

Copy link
Contributor Author

@mauro-oto mauro-oto Feb 25, 2021 •
edited
Loading

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 🤔

Copy link
Contributor Author

@mauro-oto mauro-oto Feb 25, 2021

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?

Copy link
Collaborator

@schneems schneems Feb 25, 2021

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

/unexpected end/, # Ruby 2.6
/unexpected keyword_end/i # Ruby 2.5

@error_symbol = :unmatched_syntax
else
Expand Down
52 changes: 36 additions & 16 deletions spec/unit/who_dis_syntax_error_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,25 +18,45 @@ module DeadEnd
).to eq(:end)
end

it "determines the type of syntax error to be an unmatched pipe" do
source = <<~EOM
class Blerg
Foo.call do |a
end # one
context "determines the type of syntax error to be an unmatched pipe" do
Copy link
Contributor Author

@mauro-oto mauro-oto Feb 13, 2021

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?

Copy link
Collaborator

@schneems schneems Feb 22, 2021

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.

it "with unexpected 'end'" do
source = <<~EOM
class Blerg
Foo.call do |a
end # one
puts lol
class Foo
end # two
end # three
EOM
puts lol
class Foo
end # two
end # three
EOM

expect(
DeadEnd.invalid_type(source).error_symbol
).to eq(:unmatched_syntax)
expect(
DeadEnd.invalid_type(source).error_symbol
).to eq(:unmatched_syntax)

expect(
DeadEnd.invalid_type(source).unmatched_symbol
).to eq(:|)
expect(
DeadEnd.invalid_type(source).unmatched_symbol
).to eq(:|)
end

it "with unexpected local variable or method" do
source = <<~EOM
class Blerg
[].each do |a
puts a
end
end
EOM

expect(
DeadEnd.invalid_type(source).error_symbol
).to eq(:unmatched_syntax)

expect(
DeadEnd.invalid_type(source).unmatched_symbol
).to eq(:|)
end
end

it "determines the type of syntax error to be an unmatched bracket" do
Expand Down