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

Already on GitHub? Sign in to your account

Add Style/RedundantRegexpEscape cop #7908

Conversation

@owst
Copy link
Contributor

owst commented Apr 25, 2020

This cop checks for redundant escapes inside Regexp literals.

      # @example
      #   # bad
      #   %r{foo\/bar}
      #
      #   # good
      #   %r{foo/bar}
      #
      #   # good
      #   /foo\/bar/
      #
      #   # good
      #   %r/foo\/bar/
      #
      #   # bad
      #   /a\-b/
      #
      #   # good
      #   /a-b/
      #
      #   # bad
      #   /[\+\-]\d/
      #
      #   # good
      #   /[+\-]\d/

This would have been a good use of #7868 (which I really like the sound of @marcandre - 馃憤) - but instead I used the workaround he listed of "redoing the detection work" again.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.
@owst owst force-pushed the owst:prevent_unnecessarily_escaped_slashes_in_percent_r_regexps branch from d0abd9d to cc1b1ce Apr 25, 2020
Copy link
Collaborator

jonas054 left a comment

I suggest this functionality be placed in a separate new cop. It's reasonable for a user to not care about which delimiters are used, and thus disable Style/RegexpLiteral, but still want to get rid of unnecessary escaping.

@@ -5,6 +5,7 @@
### New features

* [#7895](https://github.com/rubocop-hq/rubocop/pull/7895): Include `.simplecov` file by default. ([@robotdana][])
* [#7908](https://github.com/rubocop-hq/rubocop/pull/7908): Add unnecessary `/`-escapes offence to `Style/RegexpLiteral`. ([@owst][])

This comment has been minimized.

Copy link
@jonas054

jonas054 May 24, 2020

Collaborator

offenses is the spelling we use.

@owst
Copy link
Contributor Author

owst commented May 27, 2020

Thanks for the review @jonas054 and good suggestion for the separate cop - I've made the change, which allowed me to also catch a lot of other unnecessary escapes, when you have a chance, let me know what you think 馃憤

owst added 2 commits May 28, 2020
鈥ily_escaped_slashes_in_percent_r_regexps
Copy link
Collaborator

jonas054 left a comment

This is much better! I had some questions and comments.


MSG_REDUNDANT_ESCAPE = 'Redundant escape inside regexp literal'

ALLOWED_ALWAYS_ESCAPES = ' []^\\#'.chars.freeze

This comment has been minimized.

Copy link
@jonas054

jonas054 May 30, 2020

Collaborator

I'm not sure why the space character is always allowed to escape. Is it when you use the x flag?

This comment has been minimized.

Copy link
@owst

owst May 30, 2020

Author Contributor

Yes, exactly. I could add another explicit message that said "Unnecessary escape of space, when not using free-spacing (x) mode."?

MSG_REDUNDANT_ESCAPE = 'Redundant escape inside regexp literal'

ALLOWED_ALWAYS_ESCAPES = ' []^\\#'.chars.freeze
ALLOWED_WITHIN_CHAR_CLASS_METACHAR_ESCAPES = '-'.chars.freeze

This comment has been minimized.

Copy link
@jonas054

jonas054 May 30, 2020

Collaborator

You can always put a dash at the beginning or end of a character class and not have to escape it, right? It would require more logic in the cop, though.

This comment has been minimized.

Copy link
@owst

owst May 30, 2020

Author Contributor

Yes, that's true - it's similar to how ^ only needs to be escaped if it is the first character of a character class.

My two reasons for not flagging/removing such escapes is that it would involve more logic as you say, and make the resulting character classes slightly more "fragile" to changes over time - either by adding another character or by reordering the elements of the character class, e.g.:

[a-] => [a-b]
[-a] => [b-a]
[ab-] => [a-b]
[a^] => [^a]

would all change the meaning of the character class. The refactorings may not be common, but I think I fall down on allowing escapes of ^ and - even though they are technically redundant. What do you think? I can take a look at the implementation would be if you think it's worth flagging such cases.

Style/RedundantRegexpEscape:
Description: 'Checks for redundant escapes in Regexps.'
Enabled: true
VersionAdded: '0.85'

This comment has been minimized.

Copy link
@jonas054

jonas054 May 30, 2020

Collaborator

I'm not sure if the next version will be 0.85, or if it will be 1.0. @bbatsov knows.

This comment has been minimized.

Copy link
@bbatsov

bbatsov May 31, 2020

Collaborator

Seems it will be 0.85. Too much development happening recently. I want to give @marcandre time to finish the things he has in progress.


Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
--- | --- | --- | --- | ---
Enabled | Yes | Yes | 0.83 | -

This comment has been minimized.

Copy link
@jonas054

jonas054 May 30, 2020

Collaborator

Looks like the file needs to be generated again, looking at the "version added".

This comment has been minimized.

Copy link
@owst

owst May 30, 2020

Author Contributor

Oops, yes, good catch, I missed this after I merged master and bumped the VersionAdded you noted above - I'll regenerate now and can change again when @bbatsov advises about the next version number

@owst owst changed the title Add offence for unnecessary `/`-escapes in Style/RegexpLiteral Add Style/RedundantRegexpEscape cop May 30, 2020
@owst owst mentioned this pull request May 30, 2020
8 of 8 tasks complete
Copy link
Collaborator

jonas054 left a comment

Both your remaining questions are about false negatives. Things we could warn about but don't. I think that's OK, and can be improved later. Everything looks good to me.

@bbatsov bbatsov merged commit 3d064b4 into rubocop-hq:master May 31, 2020
23 of 25 checks passed
23 of 25 checks passed
windows 2.4
Details
windows 2.5
Details
windows 2.6
Details
windows 2.7 windows 2.7
Details
windows mingw
Details
ci/circleci: jruby-9.2-spec CircleCI is running your tests
Details
ci/circleci: cc-setup Your tests passed on CircleCI!
Details
ci/circleci: documentation-checks Your tests passed on CircleCI!
Details
ci/circleci: jruby-9.2-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: jruby-9.2-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.6-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.6-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.6-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.7-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.7-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.7-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-head-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-head-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-head-spec Your tests passed on CircleCI!
Details
@bbatsov
Copy link
Collaborator

bbatsov commented May 31, 2020

Great work!

koic added a commit to rubocop-hq/rubocop-performance that referenced this pull request May 31, 2020
Follow up to rubocop-hq/rubocop#7908.
koic added a commit to rubocop-hq/rubocop-rails that referenced this pull request May 31, 2020
Follow up to rubocop-hq/rubocop#7908.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can鈥檛 perform that action at this time.