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

Escape regex properly #275

Merged
merged 1 commit into from Oct 17, 2018

Conversation

Projects
None yet
4 participants
@Aerijo
Contributor

Aerijo commented Sep 20, 2018

Description of the Change

Add in escape rule, so we don't get false positives for groups, etc.

Alternate Designs

I could have tried finding all proper escapable characters, but that's difficult. My experiments with regex101.com indicate escaping any character will count as an escape, in some way or another (e.g., \q means a literal q).

Benefits

No more leaking regex when opening parentheses are supposed to be escaped

Possible Drawbacks

Unicode escapes are more complicated, but I believe they are not supported at all currently. This will not break them.

Applicable Issues

Fixes #259

@kylebarron

This comment has been minimized.

Contributor

kylebarron commented Sep 21, 2018

It seems that both this and #269 would be beneficial, while each claim to solve #259.

@Aerijo

This comment has been minimized.

Contributor

Aerijo commented Sep 21, 2018

@kylebarron I believe #269 is redundant with this PR. It also fails to match \\( as the start of a group, and lacks tests.

@kylebarron

This comment has been minimized.

Contributor

kylebarron commented Sep 21, 2018

Sounds good. I didn't look at it too closely, just wanted to draw attention to the fact that it also claimed to solve the issue.

@lee-dohm

This comment has been minimized.

Member

lee-dohm commented Sep 25, 2018

@50Wliu would you mind taking a look here and giving your thoughts?

@asheren asheren referenced this pull request Oct 16, 2018

Closed

Iteration Plan: October 15 - October 26 #18237

0 of 15 tasks complete
@daviwil

This comment has been minimized.

Member

daviwil commented Oct 17, 2018

I just tested this change out locally on my machine and it seems to work perfectly. Nice work @Aerijo! Tests look good too, so I'll go ahead and merge this and get the version updated.

@daviwil daviwil merged commit a04466b into atom:master Oct 17, 2018

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