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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 Properly handle variables ending in "do" followed by pipes #215

Merged
merged 1 commit into from Sep 23, 2017

Conversation

Projects
None yet
2 participants
@stevenpetryk

stevenpetryk commented Sep 23, 2017

Description of the Change

Variables ending in "do" followed by || or ||= were mistakenly treated as blocks by the grammar:

The grammar was naively treating do | as a block. This PR just adds a few extra checks to ensure that do is not immediately preceded by an identifier character.

Tada!

Alternate Designs

It would be nice if this could be done using a more sane regex, but that is impossible since a lookbehind is necessary. Imagine if it were this:

\b(do|{)\s*|

Wouldn't that be nice!

Benefits

Variables ending in do will not be confused with blocks. This is especially great for Spanish variable names.

Possible Drawbacks

Can't imagine any, this is just a syntax.

Applicable Issues

Fixes #214.

Steven Petryk
馃悰 Properly handle variables ending in "do" followed by pipes
Fixes #214.

Variables ending in "do" followed by || or ||= were mistakenly treated
as blocks by the grammar:

  sudo ||= true
  avocado || watermelon

The grammar was naively treating `do |` as a block. This PR just adds a
few extra checks to ensure that `do` is not immediately preceded by an
identifier character.
@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Sep 23, 2017

Member

Ouch...looks like I'll have to take some time to clean these regexes up.

Member

50Wliu commented Sep 23, 2017

Ouch...looks like I'll have to take some time to clean these regexes up.

@@ -1860,7 +1860,7 @@
]
}
{
'begin': '(?<={|do|{\\s|do\\s)(\\|)'
'begin': '(?<={|{\\s|[^A-Za-z0-9_]do|^do|[^A-Za-z0-9_]do\\s|^do\\s)(\\|)'

This comment has been minimized.

@50Wliu

50Wliu Sep 23, 2017

Member

This is absolutely horrible, but it seems like the best solution without a large rewrite. Thanks for the PR.

@50Wliu

50Wliu Sep 23, 2017

Member

This is absolutely horrible, but it seems like the best solution without a large rewrite. Thanks for the PR.

@50Wliu 50Wliu merged commit ef4eb56 into atom:master Sep 23, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment