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

#11514 New cop to catch unescaped ']' in RegExp #12805

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Andy-Stack
Copy link

This pull request adds a new cop that will catch and provide an auto-fix for unescaped closing square bracket meta characters in a regular expression.

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 25, 2024

I wonder if the cop is not way too specific in its current form and shouldn't be expand to account for other similar scenarios.

@Andy-Stack
Copy link
Author

Happy to try and expand this. Did you have anything in mind?

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 8, 2024

@owst may have some suggestions. I recall he used to work on some Regexp related checks.

@owst
Copy link
Contributor

owst commented Apr 9, 2024

@owst may have some suggestions. I recall he used to work on some Regexp related checks.

Yeah, this cop feels closely related to Style/RedundantRegexpEscape.

Having poked about a bit, I think Ruby will warn for an unescaped ] unless it appears as the first character in the pattern (tested with 3.3.0):

$ ruby -e 'p /]/'
/]/
$ ruby -e 'p /]X/'
/]X/
$ ruby -e 'p /X]/'
-e:1: warning: regular expression has ']' without escape: /X]/
/X]/

I agree with @bbatsov - this change feels overly specific at least as-is.

However, perhaps the behaviour in this PR could be achieved by adding a style to the existing cop (perhaps after renaming it?) that allows/requires ] to be escaped even when it is redundant to do so (at the start of the pattern). Similarly, as per #11152 escaping a - is redudant at the start or end of a character class, but I think a similar argument could be made to always require - to be escaped within a character class. There are possibly other cases, too.

As a name to improve on, how about a style name of require_position_dependent to require escapes that are only redundant in certain positions?

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

Successfully merging this pull request may close these issues.

None yet

3 participants