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

Relax alignment requirements #7

Closed
codener opened this issue Mar 15, 2019 · 4 comments
Closed

Relax alignment requirements #7

codener opened this issue Mar 15, 2019 · 4 comments

Comments

@codener
Copy link
Member

codener commented Mar 15, 2019

This little method tries to parse a date from request parameters and falls back to "today":

def set_date
  @date = begin
    Date.parse(params[:date])
  rescue TypeError, ArgumentError
    Date.today
  end
end

However, Rubocop complains that rescue doesn't align with begin. While this satiesfies it (WAT) ...

def set_date
  @date = begin
    Date.parse(params[:date])
          rescue TypeError, ArgumentError
            Date.today
  end
end

... probably this is what Rubocop wants:

def set_date
  @date = begin
            Date.parse(params[:date])
          rescue TypeError, ArgumentError
            Date.today
          end
  end

This huge indentation is no good thing. It denies the "indent with 2 spaces" principle and worse, it makes many lines depend on a single other line: when the @date variable is renamed, all the lines below need to be rearranged.

Verdict

Rubocop should not force begin and rescue to align. There's nothing wrong in the first code example.

Probably there are related alignment rules that need to be adapted similarly.

@foobear
Copy link
Member

foobear commented Mar 15, 2019

To clarify: The cop in question is Layout/RescueEnsureAlignment.

And yes, you are right, Rubocop's layout cops default to aligning the entire statement, as outlined in your 3rd code block (which is a preferred way for many Rubyists). While the 2nd code block satisfies the cop, we definitely would not want that.

The cop's actual intention is still valid: rescue must be outdented. We would want to enforce this since there is never a reason to indent rescue like the code it rescues errors from.
However, the cop is missing an alignment option like variable for Layout/EndAlignment.

Since this is an issue in Rubocop, I agree we should disable the cop for now.
Please open an issue on the rubocop repo to introduce an extra option to support variable-level assignment in future versions.

If you suggest disabling further cops, please name them explicitly. Other cops, like EndAlignment, work as required and we would not want to disable them unless they cause similar issues.

@codener
Copy link
Member Author

codener commented Mar 18, 2019

There is an issue on this topic rubocop/rubocop#6254. While its fix was released in 0.64 rubocop/rubocop#6753, it still has problems which haven't been fixed yet rubocop/rubocop#6771.

Need to have an eye on this and update our fork once that thing is straight.

@foobear
Copy link
Member

foobear commented Apr 3, 2019

Cop has been disabled in 3.1.0

@foobear
Copy link
Member

foobear commented Apr 3, 2019

Once Rubocop's related issues have been resolved, we will open a new issue for discussion if we want to re-enable the cop with a different configuration.

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

No branches or pull requests

2 participants