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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rails/HttpStatus - disable cop #363

Merged
merged 2 commits into from Oct 3, 2019
Merged

Rails/HttpStatus - disable cop #363

merged 2 commits into from Oct 3, 2019

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Jul 29, 2019

Proposing to disable the Rails/HttpStatus cop

The current setting for the cop is to prefer symbols, which means:

# "bad"
render :foo, status: 200
render json: { foo: 'bar' }, status: 200
render plain: 'foo/bar', status: 304
redirect_to root_url, status: 301

# good
render :foo, status: :ok
render json: { foo: 'bar' }, status: :ok
render plain: 'foo/bar', status: :not_modified
redirect_to root_url, status: :moved_permanently

I consider the numeric codes more readable and clearer,
but would not go as far as forcing people to use them.

(Translation table between actual status codes and rails symbols: https://guides.rubyonrails.org/layouts_and_rendering.html#the-status-option)

=> just disabling the cop in this PR

Please vote on what you prefer with 馃憤 or 馃憥 on this comment. (馃憤 means disable the cop, 馃憥 means :status => :ok is the only right way)

numeric status codes are explicit,
symbols just add an extra lookup
@himdel himdel added the vote label Jul 29, 2019
@himdel
Copy link
Contributor Author

himdel commented Jul 29, 2019

Alternative proposal in this comment:

Don't disable the cop, switch the preferred style to numeric, making :status => :ok bad, and :status => 200 the only right way.

Please vote on what you prefer with +1 or -1 on this comment.
(:+1: means switch to numeric, 馃憥 means don't switch to numeric)

@NickLaMuro
Copy link
Member

NickLaMuro commented Jul 29, 2019

@himdel honestly with the addendum comment, this is pretty confusing as people have voted, since some have voted in both comments and the original post. However, I see those two options as mutually exclusive and it is uncertain what vote counts for what. Personally, I think polling should be done in a single spot (though I guess I am indifferent if you use talk.manageiq.org or something else), but don't split the decision between two comments.

Example: What I did here for array literals: #333


I personally voted for going with enforcing numeric only over using the symbol form as :ok and friends is pretty vague. In general, I would prefer that we are consistent throughout our applications instead of "whatever the developer feels like at the time" (which then begs the question if rubocop is serving any purpose at that point...), so I think the original suggestion doesn't make sense.

But the above is also why your voting system is unclear as it is hard to tell if what you have is:

"choose between a, b, or c"

or

"choose a or b, and if a, pick a1 or a2"

or something else entirely...

@himdel
Copy link
Contributor Author

himdel commented Jul 29, 2019

..Yeah, sorry about that, but I think it doesn't have to be mutually exclusive:

parse the root comment as "do I want rubocop to NOT complain when I use numbers?",
and the addendum as "do I want rubocop to complain when I use symbols?"

@himdel
Copy link
Contributor Author

himdel commented Jul 29, 2019

Either way, looking at the current code:

manageiq-ui-classic:
symbol - 2
number - 10

manageiq-api:
symbol - 0
number - 1

@Fryguy
Copy link
Member

Fryguy commented Aug 2, 2019

@himdel If you haven't already can you create a talk article linking back to this issue? That way devs that don't follow guides can at least find out about this vote.

@himdel
Copy link
Contributor Author

himdel commented Aug 5, 2019

Good idea, thanks :)

http://talk.manageiq.org/t/vote-rubocop-rails-httpstatus-disable-cop-or-switch-to-prefer-numeric/4486

(Also updated the PR to switch to numeric, to reflect the current vote status.)

consistency good, symbolic bad :)
@Fryguy
Copy link
Member

Fryguy commented Oct 3, 2019

Looks like there is agreement to enforce numeric, so I'm going to merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants