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

Rename Rails department to RSpecRails #834

Closed
wants to merge 2 commits into from

Conversation

ixti
Copy link

@ixti ixti commented Oct 27, 2019

Rubocop provides Rails/HttpStatus cop to lint render (and its friends) methods in controllers. That causes conflicts with RSpec/Rails/HttpStatus and makes one of them dysfunctional depending on require order.

This patchset renames RuboCop::Cop::RSpec::Rails::HttpStatus to RuboCop::Cop::RSpec::RSpecRails::HttpStatus, which both resolves namespacing conflict and makes department name better match with gem it aims to (rspec-rails).


Resolves: #611

Copy link
Member

@pirj pirj 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 to me.
Thanks a lot!

.rubocop.yml Outdated Show resolved Hide resolved
Add RSpecRails -> rspec_rails pair.
@ixti ixti requested a review from pirj October 29, 2019 02:36
@ixti
Copy link
Author

ixti commented Nov 1, 2019

@bquorning any chance to get this merged? :D

@bquorning
Copy link
Collaborator

I’m split on this one. It seems like a fix of the symptoms instead of a fix of the underlying problem. RuboCop::Cop::RSpec::Rails::HttpStatus and RuboCop::Cop::Rails::HttpStatus are two different namespaces, so it should be possible to make it work.

@Darhazer and @dgollahon, do you have any input on this?

@ixti
Copy link
Author

ixti commented Nov 2, 2019

I agree that these are two different namespaces. And I had proposal of overriding default cop badge:
#611 (comment) as for what it worth I'm fine with either way. It's just right now either rails cop won't work or rspec cop won't, and it's a bummer.

@ixti
Copy link
Author

ixti commented Dec 26, 2019

Is there still interest in fixing this cop? Right now it causes a lot of frustration: depending on the order of requires either one cop will be working or the other...

@dgollahon
Copy link
Contributor

I’m split on this one. It seems like a fix of the symptoms instead of a fix of the underlying problem. RuboCop::Cop::RSpec::Rails::HttpStatus and RuboCop::Cop::Rails::HttpStatus are two different namespaces, so it should be possible to make it work.

@Darhazer and @dgollahon, do you have any input on this?

It seems like the ideal approach would be to fix the way departments/plugins are handled (presumably in rubocop) proper, but I doubt that will get resolved in a timely manner unless @ixti wants to pursue that further. The rename seems sensible in the short-term to me, except that renaming the cop will be disruptive to existing users. That said, it would be the same disruption if we supported sub-departments in the cop name because the config would still have to be updated.

I defer to @bquorning / @Darhazer, but this strikes me as reasonable for now.

@pirj
Copy link
Member

pirj commented Jan 3, 2020

After giving it another thought I tend to side with you guys, it needs to be fixed on RuboCop side.
@bbatsov I could only find rubocop/rubocop#5251, should we file another ticket to track this problem?

@bbatsov
Copy link
Contributor

bbatsov commented Jan 4, 2020

@pirj Yeah, probably we should.

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.

Rails/HttpStatus not working after requiring rubocop-rspec
5 participants