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

Do not replace and with && after a redirect_to or redirect_back #210

Closed
fidalgo opened this issue Mar 17, 2020 · 5 comments · Fixed by #224
Closed

Do not replace and with && after a redirect_to or redirect_back #210

fidalgo opened this issue Mar 17, 2020 · 5 comments · Fixed by #224

Comments

@fidalgo
Copy link
Contributor

fidalgo commented Mar 17, 2020

When writing the code:

redirect_to(root_path) and return it gets autocorrected with redirect_to(root_path) && return

same for redirect_back

This led to different behaviours, because && and and are different.

RuboCop version

$ [bundle exec] rubocop -V
0.80.0 (using Parser 2.7.0.2, running on ruby 2.6.5 x86_64-linux)

@andyw8
Copy link
Contributor

andyw8 commented Mar 17, 2020

Can you describe the incorrect behaviour you're seeing?

&& has higher precedence than and, but since you've wrapped the argument in parentheses, I would expect it to work correctly in this case.

@fidalgo
Copy link
Contributor Author

fidalgo commented Mar 17, 2020

@andyw8 Thanks for reaching out, because of double rendering errors, as you can find here https://stackoverflow.com/a/1426835/1006863 and a more in depth explanation https://guides.rubyonrails.org/layouts_and_rendering.html#avoiding-double-render-errors

@andyw8
Copy link
Contributor

andyw8 commented Mar 18, 2020

Yes, I understand the need for it, but I would have expected these to have equivalent behaviour:

redirect_to(root_path) and return
redirect_to(root_path) && return

@fidalgo
Copy link
Contributor Author

fidalgo commented Mar 20, 2020

@andyw8 not really:

Make sure to use and return instead of && return because && return will not work due to the operator precedence in the Ruby Language.

from https://guides.rubyonrails.org/layouts_and_rendering.html#avoiding-double-render-errors

also check: http://www.virtuouscode.com/2010/08/02/using-and-and-or-in-ruby/

koic added a commit to koic/rubocop-rails that referenced this issue Mar 29, 2020
…dOr`

Resolves rubocop#210.

The following idioms exist for `return` and` raise` in Ruby.

- `do_something and return`
- `do_something || raise`

And Rails will also show users the error message using this idiom.

> `"redirect_to(...) and return\"`

https://github.com/rails/rails/blob/v6.0.2.2/actionpack/lib/abstract_controller/rendering.rb#L10

This is the same for `render :action and return` and others.

`Style/AndOr` cop default rule (`EnforcedStyle: always`) does seem to
unmatch for these cases. I think these cases need to be accepted.

So, this PR changes it to `EnforcedStyle: conditionals` at least in Rails.
The enforced style cannot catch all cases, but probably the closest to
solving the issue.
@koic
Copy link
Member

koic commented Mar 29, 2020

Make sure to use and return instead of && return because && return will not work due to the operator precedence in the Ruby Language.

I agree with this one.

The following idioms exist for return and raise in Ruby.

  • do_something and return
  • do_something || raise

And Rails will also show users the error message using this idiom.

"redirect_to(...) and return\"

https://github.com/rails/rails/blob/v6.0.2.2/actionpack/lib/abstract_controller/rendering.rb#L10

Style/AndOr cop default rule (EnforcedStyle: always) does seem to unmatch for these cases. So I think these cases need to be accepted and I opened PR #224.

koic added a commit to koic/rubocop-rails that referenced this issue Apr 1, 2020
…dOr`

Resolves rubocop#210.

The following idioms exist for `return` and` raise` in Ruby.

- `do_something and return`
- `do_something || raise`

And Rails will also show users the error message using this idiom.

> `"redirect_to(...) and return\"`

https://github.com/rails/rails/blob/v6.0.2.2/actionpack/lib/abstract_controller/rendering.rb#L10

This is the same for `render :action and return` and others.

`Style/AndOr` cop default rule (`EnforcedStyle: always`) does seem to
unmatch for these cases. I think these cases need to be accepted.

So, this PR changes it to `EnforcedStyle: conditionals` at least in Rails.
The enforced style cannot catch all cases, but probably the closest to
solving the issue.
koic added a commit to koic/rubocop-rails that referenced this issue Apr 3, 2020
…dOr`

Resolves rubocop#210.

The following idioms exist for `return` and` raise` in Ruby.

- `do_something and return`
- `do_something || raise`

And Rails will also show users the error message using this idiom.

> `"redirect_to(...) and return\"`

https://github.com/rails/rails/blob/v6.0.2.2/actionpack/lib/abstract_controller/rendering.rb#L10

This is the same for `render :action and return` and others.

`Style/AndOr` cop default rule (`EnforcedStyle: always`) does seem to
unmatch for these cases. I think these cases need to be accepted.

So, this PR changes it to `EnforcedStyle: conditionals` at least in Rails.
The enforced style cannot catch all cases, but probably the closest to
solving the issue.
koic added a commit to koic/rubocop-rails that referenced this issue Apr 14, 2020
…dOr`

Resolves rubocop#210.

The following idioms exist for `return` and` raise` in Ruby.

- `do_something and return`
- `do_something || raise`

And Rails will also show users the error message using this idiom.

> `"redirect_to(...) and return\"`

https://github.com/rails/rails/blob/v6.0.2.2/actionpack/lib/abstract_controller/rendering.rb#L10

This is the same for `render :action and return` and others.

`Style/AndOr` cop default rule (`EnforcedStyle: always`) does seem to
unmatch for these cases. I think these cases need to be accepted.

So, this PR changes it to `EnforcedStyle: conditionals` at least in Rails.
The enforced style cannot catch all cases, but probably the closest to
solving the issue.
koic added a commit to koic/rubocop-rails that referenced this issue Apr 25, 2020
…dOr`

Resolves rubocop#210.

The following idioms exist for `return` and` raise` in Ruby.

- `do_something and return`
- `do_something || raise`

And Rails will also show users the error message using this idiom.

> `"redirect_to(...) and return\"`

https://github.com/rails/rails/blob/v6.0.2.2/actionpack/lib/abstract_controller/rendering.rb#L10

This is the same for `render :action and return` and others.

`Style/AndOr` cop default rule (`EnforcedStyle: always`) does seem to
unmatch for these cases. I think these cases need to be accepted.

So, this PR changes it to `EnforcedStyle: conditionals` at least in Rails.
The enforced style cannot catch all cases, but probably the closest to
solving the issue.
@koic koic closed this as completed in #224 Apr 25, 2020
koic added a commit that referenced this issue Apr 25, 2020
[Fix #210] Change enforced style to conditionals for `Style/AndOr`
koic added a commit to koic/rubocop that referenced this issue Sep 8, 2021
This PR marks `Style/AndOr` as unsafe auto-correction.

cf: rubocop/rubocop-rails#210
bbatsov pushed a commit to rubocop/rubocop that referenced this issue Sep 9, 2021
This PR marks `Style/AndOr` as unsafe auto-correction.

cf: rubocop/rubocop-rails#210
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 a pull request may close this issue.

3 participants