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

[Fix #3629] Add TargetRailsVersion functionality #4107

Merged
merged 1 commit into from Mar 15, 2017

Conversation

Projects
None yet
3 participants
@maxbeizer
Contributor

maxbeizer commented Mar 11, 2017

As described in #3629 , this
adds the option to add a TargetRailsVersion to the rubocop.yml under
AllCops. Thus, Rails-5-specific cops do not have to be manually
disengaged.

The main problem this solves is when adding RuboCop to an older Rails 4
project, running the autocorrect will actually break the specs without
much indication why.

There is a fair bit more duplication (specifically of specs) than I'd
usually like, but I figured it is better to refactor once this PR gets a
+1 from maintainers.


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).
  • Used the same coding conventions as the rest of the project.
  • 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.
  • All tests are passing.
  • The new code doesn't generate RuboCop offenses.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Updated cop documentation with rake generate_cops_documentation (required only when you've added a new cop or changed the configuration/documentation of an existing cop).
@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Mar 11, 2017

Collaborator

The cop looks good to me. I think some cops are Rails 4 only, so this might also be denoted in them, although probably few people are running Rails 3 in production these days.

Collaborator

bbatsov commented Mar 11, 2017

The cop looks good to me. I think some cops are Rails 4 only, so this might also be denoted in them, although probably few people are running Rails 3 in production these days.

potomak added a commit to tomatoes-app/tomatoes that referenced this pull request Mar 12, 2017

Add reference to blocker issue
When rubocop-hq/rubocop#4107 is completed it will
be possible to remove this exclusion. This exclusion is here only
because the `Rails/HttpPositionalArguments` cop doesn't work in Rails 4.
Show outdated Hide outdated lib/rubocop/config.rb Outdated
Show outdated Hide outdated lib/rubocop/config.rb Outdated
@maxbeizer

This comment has been minimized.

Show comment
Hide comment
@maxbeizer

maxbeizer Mar 13, 2017

Contributor

Requested changes made. I'm happy to squash it all down to one commit when we are all 👍 .

I think some cops are Rails 4 only, so this might also be denoted in them, although probably few people are running Rails 3 in production these days.

I'm not totally sure which cops this would apply to. The only one I came across what the http verb cop. Are there any others I should look at updating?

Contributor

maxbeizer commented Mar 13, 2017

Requested changes made. I'm happy to squash it all down to one commit when we are all 👍 .

I think some cops are Rails 4 only, so this might also be denoted in them, although probably few people are running Rails 3 in production these days.

I'm not totally sure which cops this would apply to. The only one I came across what the http verb cop. Are there any others I should look at updating?

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Mar 13, 2017

Collaborator

I'm not totally sure which cops this would apply to. The only one I came across what the http verb cop. Are there any others I should look at updating?

This is certainly Rails 4+ https://rubocop.readthedocs.io/en/latest/cops_rails/#railsactionfilter

Not sure about the rest. Guess someone should consult the Rails 4 changelog as it happened a while ago and I don't remember what else was added then.

Collaborator

bbatsov commented Mar 13, 2017

I'm not totally sure which cops this would apply to. The only one I came across what the http verb cop. Are there any others I should look at updating?

This is certainly Rails 4+ https://rubocop.readthedocs.io/en/latest/cops_rails/#railsactionfilter

Not sure about the rest. Guess someone should consult the Rails 4 changelog as it happened a while ago and I don't remember what else was added then.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Mar 14, 2017

Collaborator

Squash the related commits together and we're good go to.

Collaborator

bbatsov commented Mar 14, 2017

Squash the related commits together and we're good go to.

[Fix #3629] Add TargetRailsVersion functionality
As described in #3629 , this
adds the option to add a TargetRailsVersion to the rubocop.yml under
AllCops. Thus, Rails-5-specific cops do not have to be manually
disengaged.

The main problem this solves is when adding RuboCop to an older Rails 4
project, running the autocorrect will actually break the specs without
much indication why.

There is a fair bit more duplication (specifically of specs) than I'd
usually like, but I figured it is better to refactor once this PR gets a
+1 from maintainers.
@maxbeizer

This comment has been minimized.

Show comment
Hide comment
@maxbeizer

maxbeizer Mar 14, 2017

Contributor

I opted to skip the cop if using Rails < 4.0. All commits fixed up and rebased master

Contributor

maxbeizer commented Mar 14, 2017

I opted to skip the cop if using Rails < 4.0. All commits fixed up and rebased master

@bbatsov bbatsov merged commit 702a403 into rubocop-hq:master Mar 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Mar 15, 2017

Collaborator

👍

Collaborator

bbatsov commented Mar 15, 2017

👍

jsugarman added a commit to ministryofjustice/peoplefinder that referenced this pull request May 25, 2017

bump rubocop to 0.48.1 --> 0.49.0
Introduces TargetRailsVersion which is needed to
avoid unnecessary rails 5.0 cops. Specifically,
[RuboCop::Cop::Rails::HttpPositionalArguments](http://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Rails/HttpPositionalArguments)

see [Add TargetRailsVersion functionality](rubocop-hq/rubocop#4107)

jsugarman added a commit to ministryofjustice/peoplefinder that referenced this pull request May 25, 2017

bump rubocop to 0.48.1 --> 0.49.0
Introduces TargetRailsVersion which is needed to
avoid unnecessary rails 5.0 cops. Specifically,
[RuboCop::Cop::Rails::HttpPositionalArguments](http://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Rails/HttpPositionalArguments)

see [Add TargetRailsVersion functionality](rubocop-hq/rubocop#4107)

jsugarman added a commit to ministryofjustice/peoplefinder that referenced this pull request May 26, 2017

bump rubocop to 0.48.1 --> 0.49.0
Introduces TargetRailsVersion which is needed to
avoid unnecessary rails 5.0 cops. Specifically,
[RuboCop::Cop::Rails::HttpPositionalArguments](http://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Rails/HttpPositionalArguments)

see [Add TargetRailsVersion functionality](rubocop-hq/rubocop#4107)

jsugarman added a commit to ministryofjustice/peoplefinder that referenced this pull request May 26, 2017

bump rubocop to 0.48.1 --> 0.49.0
Introduces TargetRailsVersion which is needed to
avoid unnecessary rails 5.0 cops. Specifically,
[RuboCop::Cop::Rails::HttpPositionalArguments](http://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Rails/HttpPositionalArguments)

see [Add TargetRailsVersion functionality](rubocop-hq/rubocop#4107)

jsugarman added a commit to ministryofjustice/peoplefinder that referenced this pull request Jun 6, 2017

bump rubocop to 0.48.1 --> 0.49.0
Introduces TargetRailsVersion which is needed to
avoid unnecessary rails 5.0 cops. Specifically,
[RuboCop::Cop::Rails::HttpPositionalArguments](http://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Rails/HttpPositionalArguments)

see [Add TargetRailsVersion functionality](rubocop-hq/rubocop#4107)

@barodeur barodeur referenced this pull request Nov 1, 2017

Closed

[Fix #4979] Disable Rails/FindBy when using rails3 #4980

11 of 11 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment