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

Add new Lint/DuplicateRegexpCharacterClassElement cop #8896

Merged
merged 5 commits into from Oct 26, 2020

Conversation

owst
Copy link
Contributor

@owst owst commented Oct 15, 2020

Cop documentation:

      # This cop checks for duplicate elements in Regexp character classes.
      #
      # @example
      #
      #   # bad
      #   r = /[xyx]/
      #
      #   # bad
      #   r = /[0-9x0-9]/
      #
      #   # good
      #   r = /[xy]/
      #
      #   # good
      #   r = /[0-9x]/

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 duplicate_regexp_char_class_elements branch from c38a45b to 52d3d2b Compare October 15, 2020 23:06
Copy link
Contributor

@marcandre marcandre left a comment

Choose a reason for hiding this comment

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

Looks good. Only thing I can see is I think this would trigger a false positive on repeated space:

hex = /[
  a-f  # lowercase letter
  0-9 # digit
]+/x

Style/DuplicateRegexpCharacterClassElement:
Description: 'Checks for duplicate elements in Regexp character classes.'
Enabled: pending
VersionAdded: '0.94'
Copy link
Collaborator

Choose a reason for hiding this comment

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

After rebasing update this to 1.1. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🎉 amazing - 1.0! Great news @bbatsov - thank you for all of your and the core team's hard work!

config/default.yml Outdated Show resolved Hide resolved
@owst
Copy link
Contributor Author

owst commented Oct 21, 2020

@marcandre regarding your comment:

Looks good. Only thing I can see is I think this would trigger a false positive on repeated space:

hex = /[
  a-f  # lowercase letter
  0-9 # digit
]+/x

Indeed, I thought the same would happen, but actually, it appears that Ruby's free-space mode doesn't support comments inside characters classes (at least on 2.6.6):

[1] pry(main)> hex = /[
[1] pry(main)*   a-f  # lowercase letter
[1] pry(main)*   0-9 # digit
[1] pry(main)* ]+/x
=> /[
  a-f  # lowercase letter
  0-9 # digit
]+/x
[2] pry(main)> hex.match('loot')
=> #<MatchData "loot">

We shouldn't be able to match loot unless # lowercase letter is not a comment.

@marcandre marcandre merged commit fd65a6b into rubocop:master Oct 26, 2020
@marcandre
Copy link
Contributor

Awesome, thanks!

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