The Wayback Machine - https://web.archive.org/web/20221207052659/https://github.com/atom/language-php/pull/114
Skip to content
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

Fix assignment operator consuming comparison operators #114

Merged
merged 1 commit into from Dec 22, 2015
Merged

Fix assignment operator consuming comparison operators #114

merged 1 commit into from Dec 22, 2015

Conversation

denis-sokolov
Copy link
Contributor

@denis-sokolov denis-sokolov commented Dec 20, 2015

=== is currently being treated as three assignment operators.

@@ -1216,6 +1212,10 @@
'name': 'keyword.operator.comparison.php'
}
{
'match': '=|\\+=|\\-=|\\*=|/=|%=|&=|\\|=|\\^=|<<=|>>='
Copy link
Member

@50Wliu 50Wliu Dec 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're at it, it looks like = should be matched last in this pattern.

@50Wliu
Copy link
Member

50Wliu commented Dec 20, 2015

Looks like some specs broke - could you please take a look at that? Thanks!

@denis-sokolov
Copy link
Contributor Author

denis-sokolov commented Dec 21, 2015

I did not know that you had a spec, since npm test did diddly squat! :)
Now that I know, I’ve added a spec for the PR and all specs are passing.

'match': '=|\\+=|\\-=|\\*=|/=|%=|&=|\\|=|\\^=|<<=|>>='
'name': 'keyword.operator.assignment.php'
}
{
'match': '(<=|>=|<|>)'
'name': 'keyword.operator.comparison.php'
Copy link
Member

@50Wliu 50Wliu Dec 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't the two comparison matches be combined into one?

Copy link
Contributor Author

@denis-sokolov denis-sokolov Dec 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because <<= needs to match before <=, but === needs to match before =.
If there's a way to do that with no two comparison matches, great.

Copy link
Member

@50Wliu 50Wliu Dec 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that is tricky. Let's just leave it as-is then :).

@50Wliu
Copy link
Member

50Wliu commented Dec 21, 2015

I did not know that you had a spec, since npm test did diddly squat! :)

Try apm test next time 😉.

@50Wliu
Copy link
Member

50Wliu commented Dec 21, 2015

One last question before I merge: is !== getting tokenized correctly? It looks like it's being overridden by ==.

@denis-sokolov
Copy link
Contributor Author

denis-sokolov commented Dec 21, 2015

Added a spec for that - apparently, no. I am not exactly sure, why.

50Wliu added a commit that referenced this pull request Dec 22, 2015
Fix assignment operator consuming comparison operators
@50Wliu 50Wliu merged commit c16c875 into atom:master Dec 22, 2015
@50Wliu
Copy link
Member

50Wliu commented Dec 22, 2015

🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants