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

Rails/HttpPositionalArguments points offenses in rails 4.2 apps #3629

Closed
hugocorbucci opened this Issue Oct 15, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@hugocorbucci

hugocorbucci commented Oct 15, 2016

It would be great for the new Rails/HttpPositionalArguments to be aware of the rails version it's inspecting to not suggest changes that incompatible with the running version.


Expected behavior

The get :method, params: { ... } syntax is only valid in rails 5 and above. However, rubocop points it as an offense to use positional arguments even in rails 4.2 applications. In those, using the get :method, params: { ... } syntax breaks tests.
I would expect Rubocop to not point out those as offenses if verifying rails 4.2 (or lower) applications.

Actual behavior

Rubocop points HttpPositionalArguments arguments but fixing them breaks the tests in Rails 4.2.

Steps to reproduce the problem

  • Create a new rails 4.2 app
  • Create a controller spec using get :show, id: 1
  • Run rubocop

RuboCop version

$ rubocop -V
0.44.1 (using Parser 2.3.1.4, running on ruby 2.3.1 x86_64-darwin15)
@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Oct 16, 2016

Collaborator

There's not support for Rails versions in cops, so right now in such cases cops should just be disabled. I'm guessing we can add a TargetRailsVersion similar to TargetRubyVersion to handle this problem.

Collaborator

bbatsov commented Oct 16, 2016

There's not support for Rails versions in cops, so right now in such cases cops should just be disabled. I'm guessing we can add a TargetRailsVersion similar to TargetRubyVersion to handle this problem.

@bbatsov bbatsov added the enhancement label Oct 16, 2016

@hugocorbucci

This comment has been minimized.

Show comment
Hide comment
@hugocorbucci

hugocorbucci Oct 16, 2016

Oh... I had no idea there was not target rails...
That seems like a reasonable amount of work then. I'll give it a shot later and try to put a PR together if you think this would be valuable.
There is an obvious workaround so not rush at all:
I disabled that specific rule (using .rubocop.yml) on my project and just have a TODO to reenable when I migrate to Rails 5.

hugocorbucci commented Oct 16, 2016

Oh... I had no idea there was not target rails...
That seems like a reasonable amount of work then. I'll give it a shot later and try to put a PR together if you think this would be valuable.
There is an obvious workaround so not rush at all:
I disabled that specific rule (using .rubocop.yml) on my project and just have a TODO to reenable when I migrate to Rails 5.

@logicminds

This comment has been minimized.

Show comment
Hide comment
@logicminds

logicminds Oct 18, 2016

Contributor

Yea I knew about this issue when I created the new cop. Best scenario I could come up with was figuring out the rails gem version using something like Gem::Specification.latest_specs and searching for the rails version. However, since rubocop is not always executed from the root of the project with bundle exec there is no promise that the correct gem would be found.

Contributor

logicminds commented Oct 18, 2016

Yea I knew about this issue when I created the new cop. Best scenario I could come up with was figuring out the rails gem version using something like Gem::Specification.latest_specs and searching for the rails version. However, since rubocop is not always executed from the root of the project with bundle exec there is no promise that the correct gem would be found.

weiqingtoh added a commit to coursemology-collab/coursemology2 that referenced this issue Dec 21, 2016

weiqingtoh added a commit to coursemology-collab/coursemology2 that referenced this issue Dec 21, 2016

guigs added a commit to ad2games/rubocop-ci that referenced this issue Dec 22, 2016

Disable Rails/HttpPositionalArguments
While our apps are still on Rails 4.2, because this rule only applies to Rails 5
rubocop-hq/rubocop#3629
@logicminds

This comment has been minimized.

Show comment
Hide comment
@logicminds

logicminds Jan 17, 2017

Contributor

So what if we added a rails version detection mechanism like so:

def rails_version
  Gem::Specification.latest_specs.find {|f| f.name == 'rails' }.version 
end

def rails5?
  rails_version >= Gem::Version.new('5')
end
Contributor

logicminds commented Jan 17, 2017

So what if we added a rails version detection mechanism like so:

def rails_version
  Gem::Specification.latest_specs.find {|f| f.name == 'rails' }.version 
end

def rails5?
  rails_version >= Gem::Version.new('5')
end
@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Jan 17, 2017

Collaborator

I still think we should go down the TargetRailsVersion route.

Collaborator

bbatsov commented Jan 17, 2017

I still think we should go down the TargetRailsVersion route.

@logicminds

This comment has been minimized.

Show comment
Hide comment
@logicminds

logicminds Jan 17, 2017

Contributor

Is there an approach or issue for that?

Contributor

logicminds commented Jan 17, 2017

Is there an approach or issue for that?

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Jan 17, 2017

Collaborator

I don't think there's a ticket but you basically need to replicate the existing functionality for TargetRubyVersion.

Collaborator

bbatsov commented Jan 17, 2017

I don't think there's a ticket but you basically need to replicate the existing functionality for TargetRubyVersion.

MaximilianMeister added a commit to MaximilianMeister/velum that referenced this issue Feb 9, 2017

LucoLe added a commit to AirHelp/ah-ruby_style_guide that referenced this issue Feb 22, 2017

Disable Rails/HttpPositionalArguments cop
This option works only for Rails 5 applications (rubocop-hq/rubocop#3629).
So I propose to disable this cop for the moment.

LucoLe added a commit to AirHelp/ah-feng_shui that referenced this issue Feb 22, 2017

Bump Ruby and disable HttpPositionalArguments
All of the projects are now migrated to ruby 2.3.3 so let's bump the
version in the rubocop config so we don't get errors for new features of
the language.

Rails/HttpPositionalArguments option works only for Rails 5 applications
(rubocop-hq/rubocop#3629). So I propose to disable
this cop for the moment.

maxbeizer added a commit to maxbeizer/rubocop that referenced this issue Mar 11, 2017

[Fix rubocop-hq#3629] Add TargetRailsVersion functionality
As described in rubocop-hq#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 maxbeizer referenced this issue Mar 11, 2017

Merged

[Fix #3629] Add TargetRailsVersion functionality #4107

11 of 11 tasks complete

maxbeizer added a commit to maxbeizer/rubocop that referenced this issue Mar 13, 2017

[Fix rubocop-hq#3629] Add TargetRailsVersion functionality
As described in rubocop-hq#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 added a commit to maxbeizer/rubocop that referenced this issue Mar 14, 2017

[Fix rubocop-hq#3629] Add TargetRailsVersion functionality
As described in rubocop-hq#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.

@bbatsov bbatsov closed this in 702a403 Mar 15, 2017

brandonweiss added a commit to brandonweiss/rubocop that referenced this issue Jul 13, 2017

[Fix rubocop-hq#3629] Add TargetRailsVersion functionality
As described in rubocop-hq#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.

gmcquistin added a commit to geome/geo_me-rubocop that referenced this issue Dec 19, 2017

satyap added a commit to brandonweiss/rubocop that referenced this issue Jan 11, 2018

[Fix rubocop-hq#3629] Add TargetRailsVersion functionality
As described in rubocop-hq#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment