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

Add consistent_relative_to_receiver to FirstParameterIndentation #5699

Merged
merged 3 commits into from
May 14, 2018
Merged

Add consistent_relative_to_receiver to FirstParameterIndentation #5699

merged 3 commits into from
May 14, 2018

Conversation

jfelchner
Copy link
Contributor

@jfelchner jfelchner commented Mar 18, 2018

There are multiple valid issues with people complaining about how FirstParameterIndentation's chosen correction is not very readable.

This pull request introduces a new style to the cop called consistent_relative_to_receiver which will always align the first parameter relative to the receiver/parent node instead of looking at the line's indentation.

Examples:

Issue #4308

Actual

MyClass.my_method(a_hash.merge(
  hello: :world,
  some: :hash,
  goes: :here
), other_arg)

vs Expected

MyClass.my_method(a_hash.merge(
                    hello: :world,
                    some: :hash,
                    goes: :here
                  ), other_arg)

Issue #5650

Actual

good3 = foo.methA(
  :arg1,
  :arg2,
  options: :hash,
)
          .methB(
            :arg1,
            :arg2,
          )
          .methC

vs Expected

good3 = foo.methA(
          :arg1,
          :arg2,
          options: :hash,
        )
        .methB(
          :arg1,
          :arg2,
        )
        .methC

Issue #2625

Actual

price = default_price * NightsCalculator.new(
  start_date,
  end_date).nights_count

vs Expected

price = default_price * NightsCalculator.new(
                          start_date,
                          end_date).nights_count

References: #5650, #4308, #3204, #2625


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run rake default or rake parallel. It executes all tests and RuboCop for itself, and generates the documentation.

@jfelchner
Copy link
Contributor Author

@bbatsov @jonas054 @rrosenblum @Drenmi if one of you could help me out, I need some assistance getting this over the finish line.

There are two tests failing on this branch. The assertions are correct. I believe that what's happening is the AlignmentCorrector is only looking at the top-most node for the indention level it should be correcting the alignment to, but in order for the tests to pass, it requires aligning based on multiple levels.

I just don't know enough about some of the Rubocop helpers/internals to know what the best approach to take is here. If someone could help me out with this, between this PR and #5698 we should be able to close out around 8-10 issues. 👍

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 21, 2018

good3 = foo.methA(
  :arg1,
  :arg2,
  options: :hash,
)
          .methB(
            :arg1,
            :arg2,
          )
          .methC

I'd say that's a bug in whatever style. :-)

I'm overloaded with work, but I'll take a look at this when I can if someone else doesn't beat me to it. We can also ask for help @garettarrowood, @pocke and @koic.

CHANGELOG.md Outdated
@@ -5,6 +5,7 @@
### New features

* [#5597](https://github.com/bbatsov/rubocop/pull/5597): Add new `Rails/HttpStatus` cop. ([@anthony-robin][])
* [#5699](https://github.com/bbatsov/rubocop/pull/5699): Add `consistent_relative_to_receiver` style option to `Layout/FirstParameterIndentation` ([@jfelchner][])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One failure is that this entry doesn't end with punctation. Adding a period will resolve it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will see if I can give the meaningful failures a closer look this weekend. I've also been swamped and haven't been able to give rubocop the attention I'd like to over the past couple months.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was speaking more about the test failures.

@jonas054
Copy link
Collaborator

@jfelchner What's happening in the failing tests is that the auto-correction of one line changes how another line needs to be aligned. This is something that we solve in RuboCop by running cops repeatedly until no more correctable offenses are found. But we can't do it within a single run of a cop.

So I recommend you change these examples to one offense only.

@jfelchner
Copy link
Contributor Author

jfelchner commented Apr 22, 2018

@bbatsov rebased onto master and force pushed with requested updates. Everything should be good to go (assuming all the specs pass). I even did a big update on the doc comments for the cop to show examples of correct formatting for all the different styles the cop accepts.

There are multiple valid issues with people complaining about how
FirstParameterIndentation's chosen correction is not very readable.

Examples:

```ruby

MyClass.my_method(a_hash.merge(
  hello: :world,
  some: :hash,
  goes: :here
), other_arg)

MyClass.my_method(a_hash.merge(
                    hello: :world,
                    some: :hash,
                    goes: :here
                  ), other_arg)
```

```ruby

good3 = foo.methA(
  :arg1,
  :arg2,
  options: :hash,
)
           .methB(
             :arg1,
             :arg2,
           )
           .methC

good3 = foo.methA(
          :arg1,
          :arg2,
          options: :hash,
        )
        .methB(
          :arg1,
          :arg2,
        )
        .methC
```

```ruby

price = default_price * NightsCalculator.new(
  start_date,
  end_date).nights_count

price = default_price * NightsCalculator.new(
                          start_date,
                          end_date).nights_count
```

This pull request introduces a new style to the cop called
`consistent_relative_to_receiver` which will always align the first
parameter relative to the receiver/parent node instead of looking at the
line's indentation.

References: #5650, #4308, #3204, #2625
@jfelchner
Copy link
Contributor Author

@bbatsov rebased to fix conflicts

@jfelchner
Copy link
Contributor Author

@bbatsov did you want additional changes here before merging?

@bbatsov bbatsov merged commit 65eb0d0 into rubocop:master May 14, 2018
@bbatsov
Copy link
Collaborator

bbatsov commented May 14, 2018

Nope. Thanks! 👍

koic added a commit to koic/rubocop that referenced this pull request Jun 9, 2018
Follow up of rubocop#4880 (comment)
and rubocop#5699.

This commit is a change of document format for `Layout/FirstParameterIndentation` cop.

This PR uses `# rubocop:disable Metrics/LineLength` because line length of
the following line exceeds 80.

```diff
+      # @example EnforcedStyle: special_for_inner_method_call_in_parentheses (default)
```

Descriptions of `EnforcedStyle` are quoted from default.yml.
https://github.com/rubocop-hq/rubocop/blob/v0.57.1/config/default.yml#L365-L381
bbatsov pushed a commit that referenced this pull request Jun 9, 2018
Follow up of #4880 (comment)
and #5699.

This commit is a change of document format for `Layout/FirstParameterIndentation` cop.

This PR uses `# rubocop:disable Metrics/LineLength` because line length of
the following line exceeds 80.

```diff
+      # @example EnforcedStyle: special_for_inner_method_call_in_parentheses (default)
```

Descriptions of `EnforcedStyle` are quoted from default.yml.
https://github.com/rubocop-hq/rubocop/blob/v0.57.1/config/default.yml#L365-L381
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.

4 participants