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 forward slash to LITERAL_REGEX #7464

Merged
merged 1 commit into from Oct 28, 2019

Conversation

eugeneius
Copy link
Contributor

The forward slash character has no special meaning when it appears in a regex. This will let Performance/StartWith and Performance/EndWith correct code like the following:

path =~ %r{/\z}

To:

path.end_with?('/')

I noticed this problem thanks to @eregon's comment here: rails/rails#37504 (comment)

LITERAL_REGEX is only used in RuboCop Performance, so I couldn't add a test to demonstrate the new behaviour. Arguably the constant should be defined there instead, but removing it from RuboCop would introduce an incompatibility with all existing versions of RuboCop Performance. I opted to update the constant in-place; let me know if another approach is preferable.


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.

The forward slash character has no special meaning when it appears in a
regex. This will let `Performance/StartWith` and `Performance/EndWith`
correct code like the following:

    path =~ %r{/\z}

To:

    path.end_with?('/')
@bbatsov
Copy link
Collaborator

bbatsov commented Oct 27, 2019

LITERAL_REGEX is only used in RuboCop Performance, so I couldn't add a test to demonstrate the new behaviour. Arguably the constant should be defined there instead, but removing it from RuboCop would introduce an incompatibility with all existing versions of RuboCop Performance. I opted to update the constant in-place; let me know if another approach is preferable.

@koic I guess we should just move this constant to the rubocop-performance project then.

@koic
Copy link
Member

koic commented Oct 28, 2019

@bbatsov Yeah. That seems good to me.

@eugeneius Could you make the constant LITERAL_REGEX to RuboCop Performance and add tests?
As you are concerned, combining RuboCop 0.75.1 or lower with RuboCop Performance 1.15.0 or lower will result in an incompatibility error.

I opted to update the constant in-place; let me know if another approach is preferable.

So, I'd like to leave this constant in RuboCop core for a while. Perhaps the constant will be removed after future release of RuboCop 0.76 and higher.
(Perhaps TracePoint, etc is used, it seems technically possible to display a warning about breaking change, but it is not a good way to do it 😅 )

@bbatsov bbatsov merged commit 18ff52e into rubocop:master Oct 28, 2019
@bbatsov
Copy link
Collaborator

bbatsov commented Oct 28, 2019

Agreed. I guess we should fix this in RuboCop for now and move the constant to rubocop-performance down the road.

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