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

Escape -? #28

Closed
benjamingr opened this issue Jun 19, 2015 · 7 comments
Closed

Escape -? #28

benjamingr opened this issue Jun 19, 2015 · 7 comments

Comments

@benjamingr
Copy link
Collaborator

Currently the proposal escapes ] and } which isn't really required since we escape [ and {. The only reason we might need to escape ] is because we need to support the case that the pattern is inserted inside the [.

A preliminary GH code search shows that this pattern is actually used. In particular, underscore's string model does "[" + escapedRegExp +"].

So, in my opinion we should support escaping inside matched sets to be on the safe side, this is somewhat wasteful (because there are other characters that do not need to be escaped in sets but the pattern is very common.

Opinions?

@benjamingr
Copy link
Collaborator Author

Also related, discussion in #17

@ljharb
Copy link
Member

ljharb commented Jun 19, 2015

I don't think there's any concern with over-escaping here. The important thing RegExp.escape provides is a safety guarantee - not a performance guarantee.

Escaping every single character might be a bit extreme, but certainly escaping the ones that might be problematic in some cases doesn't seem to be.

@domenic
Copy link
Member

domenic commented Jun 19, 2015

Maybe what we want is a table in the readme with each escaped character and a reason why it's escaped (preferably in the form new RegExp(something + RegExp.escape("the character") + something) or some subset thereof)

@benjamingr
Copy link
Collaborator Author

@domenic good idea, I'll make one soon.

@ljharb well, the most obvious reason is the readability impact, but I agree this is a use case we want to address.

@nikic
Copy link

nikic commented Jun 20, 2015

Context:

Note that this does not include -, as underscore presumably wants to allow you to write something like trim("a-zA-Z0-9") to trim all alphanumerics.

I suspect that this might be the cases in many places where something is inserted into [] and as such having - escaped there might not really be what you want.

@benjamingr
Copy link
Collaborator Author

Yes, but if you want - to work as part of a group then escape is not
what you're after anyway. You want to be able to use a user input string or
a regular string literally. You can always just concat the string as a
regular expression and not call .escape

On Sat, Jun 20, 2015 at 1:56 PM, Nikita Popov [email protected]
wrote:

Context:

Note that this does not include -, as underscore presumably wants to
allow you to write something like trim("a-zA-Z0-9") to trim all
alphanumerics.

I suspect that this might be the cases in many places where something is
inserted into [] and as such having - escaped there might not really be
what you want.


Reply to this email directly or view it on GitHub
#28 (comment)
.

@benjamingr
Copy link
Collaborator Author

Superseding this with #29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants