Introduce new AllowMultilineFinalElement option for all __LineBreak(s) cops

[Korri]

This add a AllowMultilineFinalElement option, that when set to true allows the following cops to not consider a statement to be multiline even if the last (or only) element (value, argument or parameter) itself is multiline:

FirstArrayElementLineBreak
FirstHashElementLineBreak
FirstMethodArgumentLineBreak
FirstMethodParameterLineBreak
MultilineArrayLineBreaks
MultilineHashKeyLineBreaks
MultilineMethodArgumentLineBreaks
MultilineMethodParameterLineBreaks

The goal of this addition is to be able to use Layout/LineLength to format each elements on a new line, while not forcing some of the use cases that are considered "ok exceptions" by some people.

Examples of exceptions that those changes make possible

# Allow final element to be multiline

# good
foo(a, b, bar(
  c,
  d
))

# good
{ a: b, c: {
 foo: bar,
 baz: qux,
}}

# good
[ 1, 2, [
 foo,
 bar,
]]

# Single elements are ignored too

# good
foo({
 a: b,
})

# good
a = { foo: [
  bar,
  baz,
]}

# good
b = [{
  foo: bar
}]

While still correcting the followings

# bad
foo(bar, baz,
bat)

[1, 2
3]

# good
foo(
  bar,
  baz,
  bat,
)
[
  1,
  2,
  3,
]

---

Before submitting the PR make sure the following are checked:

• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• 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.
• Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
• Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

[bbatsov]

I'm definitely not very excited by this option, as I find such indentation to be somewhat questionable but I'll consider supporting it if it's popular in the wild. @rubocop/rubocop-core Any thoughts?

Also the name of the flag can be better - e.g. AllowMultilineFinalElement or something like this. (typically our options are named AllowX in such cases.

[Korri]

I ended up trying to make that new rule as consistent and generic as possible.

The main non-starter for us, that means Shopify is not using the *LineBreak(s) cops as part of our ruby-style-guide is allowing single elements to be multi line:

# Single argument no being considered multiline:
foo({
  bar: bat,
})

# Same for single value arrays:
[foo(
   some_long_argument_name,
   with_another_one_maybe,
)]
[{
  foo: bar,
}]

# Or single value Hash (more questionable):
{ foo: {
  bar: buz
}}

Last element being multiline is more of a nice to have, but I see some legit use cases:

# this is one we see often:
foo(a, b, c, {
  foo: bar,
})

# less often, but would consider valid:
[a, b, c, {
  foo: bar,
}]

# Again more questionable for Hashes, but I have seen cases:
{ a: b, c: d, foo: {
  bar: buz,
}}

I'd be totally Ok with defining something more specific rather than the more generic and consistent across cops approach that I tried to stick too here!

[koic]

I'm definitely not very excited by this option, as I find such indentation to be somewhat questionable

Me too. Introducing this configuration will increase maintenance costs across some cop. TBH, If this rule isn't very major, it's not aggressive to add. OTOH, I'm also interested in other developer opinions that this is useful.

[Korri]

I was curious to see how ESLint handles it: looks like in most cases there is no option it's just "on" by default if you allow single line calls:

For method calls both single argument, and last argument are considered ok as multiline, they consider the following to be single line for function-call-argument-newline:

# function-call-argument-newline with the "consistent" or "never" modifier:

// good
bar("one", "two", {
    one: 1,
    two: 2
});

// good
bar({
    one: 1,
    two: 2
});

For arrays though, they only consider single element arrays to be ok on multiple lines:

// good
const a = [{
  bar: "bat"
}]

// bad
const b = ["hello", {
  bar: "bat"
}]

Objects behave the same as arrays:

// good
const a = {foo: {
  bar: "bat"
}}

// bad
const b = {a: "A", foo: {
  bar: "bat"
}}

For the function-paren-newline they call this variant multiline-arguments:

// good
bar("foo", {
    one: 1,
    two: 2
});

// good
bar({
    one: 1,
    two: 2
});

// good
bar(
  "foo",
  {
    one: 1,
    two: 2
  }
);

// bad
bar("foo",
  {
    one: 1,
    two: 2
  }
);

[Korri]

I think the ESLint behaviour would be perfect for us actually, either through an option, or by default, it doesn't have to be as flexible or generic as my pull request here!

[Korri]

In the meantime I took the advice and renamed the option, it's a much better name!

Also the name of the flag can be better - e.g. AllowMultilineFinalElement or something like this. (typically our options are named AllowX in such cases.

[bbatsov]

I'm still pondering about the best course of action here. Too many configuration options definitely seem like an overkill to me, so I'm wondering if this shouldn't be more or less a single toggle that all cops should respect. I'm not sure I want us to follow what ESLint is doing to the letter, given that the behavior so far has been different for years and it seems most people are fine with it. The indentation topics are harder than the naming topics! :-)

[bbatsov]

@dvandersluis @pirj Any thoughts on this?

[Korri]

I'm wondering if this shouldn't be more or less a single toggle that all cops should respect.

😮 is that a pattern Rubocop uses? That does definitely sounds better then having all those options!

EDIT: Oh yes I see, maybe something like ActiveSupportExtensionsEnabled 🤔 I think that could work.

[Korri]

The main downside with a global rule, is that people lose the flexibility to allow this only in certain constructs, I could see how some folks would prefer to allow this exclusively for method calls for example.
Having this allowed Hashes is probably the most controversial one (method definitions too maybe?).

I think a global rule would solve our use case though, so I'd be OK with that, we could also decide to make it more "opinionated" and not allow it for the more controversial constructs like Hashes and Arrays.

[pirj]

I may miss something with this example. It also offends Layout/ParameterAlignment which suggests changing to:

def foo(bar, baz,
        qux = false)
  do_something
end

Does it make sense to use examples that don't trigger other cops?
The primary reason I'm asking is that I'm heavily triggered by this, too, and I feel an irresistible bias against this change in general as a result.

In any case,

def foo(
bar, baz,
qux = false)
  do_something
end

or the "corrected" one below look horrible to my eye.

[Korri]

🤔 because Layout/ParameterAlignment in on by default? Those are unit tests are about a single rule. I don't think we usually care to check that other rules are not being broken by the base example here.

[Korri]

or the "corrected" one below look horrible to my eye.

🤔 yeah that rule is definitely not mean to be used in isolation. Not sure what the feedback is here though, this is the one test that is making sure that the existing behaviour is maintained for method definitions that are actually considered multiline even with AllowMultilineFinalElement on. It looks a lot like the tests that already existed in this file.

[dvandersluis]

@Korri if you run the full correction suite on the original code (with your change) how does it end up looking?

In general, it's better to not have cops do other corrections, especially when it comes to Layout because it results in duplicating a lot of work and additionally causes situations that the correct raises ClobberingErrors (the same piece of code is corrected in incompatible ways).

[Korri]

In general, it's better to not have cops do other corrections

What do you mean by "other corrections" the cop here is only adding a new line before the first parameter, it is what it's main and only role is.

@pirj for reference, the example you said "looks horrible" differs from what the autocorrect actually does, it doesn't change the indentation of the qux keyword, so:

# bad
def foo(bar, baz,
  qux = false)
  do_something
end

# get corrected to "good":
def foo(
bar, baz,
  qux = false)
  do_something
end

@Korri if you run the full correction suite on the original code (with your change) how does it end up looking?

Exactly the same as it does today, no differences! The example that I use is basically the same that is in the documentation today.

First Layout/FirstMethodParameterLineBreak is not on by default, and pretty weird to enable on it's own, as it's meant to be used at the very least in conjunction with Layout/MultilineMethodParameterLineBreaks, but let's give it a try:

# Base example (made a few tweaks to satisfy other rules, but functionally the same)
def foo(bar, baz,
  qux: false)
  do_something(bar, baz, qux)
end

# Autocorrect with the default cops set
def foo(bar, baz,
        qux: false)
  do_something(bar, baz, qux)
end


# With `Layout/FirstMethodParameterLineBreak` enabled
def foo(
  bar, baz,
  qux: false
)
  do_something(bar, baz, qux)
end

# With `Layout/FirstMethodParameterLineBreak` and `Layout/MultilineMethodParameterLineBreaks`
# enabled:
def foo(
  bar,
  baz,
  qux: false
)
  do_something(bar, baz, qux)
end

# With `Layout/FirstMethodParameterLineBreak` and `Layout/MultilineMethodParameterLineBreaks`
# and the `AllowMultilineFinalElement` true on both (no differences):
def foo(
  bar,
  baz,
  qux: false
)
  do_something(bar, baz, qux)
end

As the last point shows, there is no differences in behaviour for that specific example, based on the changes in this Pull Request, I only added that unit test to make sure that the existing behaviour is maintained, if you look higher in the test file, you will see basically the same exact test for when the option is false.

The current pull request only affects method that have their last element multiline!

[dvandersluis]

Thanks for the examples. FYI it wasn't me who you were responding to before.

What do you mean by "other corrections" the cop here is only adding a new line before the first parameter, it is what it's main and only role is.

Autocorrection for some cops tries to do layout reformatting as well. For instance, a cop that adds a new line might try to indent it to the correct level. This tends to cause issues in the long run, and is redundant because we have cops that are already present to reflow the layout when code is changed/added.

[Korri]

FYI it wasn't me who you were responding to before.

Hoo totally missed that sorry, will edit to make it clearer!

Autocorrection for some cops tries to do layout reformatting as well. For instance, a cop that adds a new line might try to indent it to the correct level. This tends to cause issues in the long run, and is redundant because we have cops that are already present to reflow the layout when code is changed/added.

Sounds like this cop is doing the right thing then :) It's not trying to fix layout, only focusing on its one job: Adding a new line.

[Korri]

Hey folks. Would love some more input on this! I'm happy to work on a more opinionated/restricted PR too if that's how we prefer it! Or a global rule too!

[bbatsov]

I'll try to make a decision about the next steps by the end of next week.

[Korri]

@bbatsov so what's the decision on this? 😄

[Korri]

After some digging here is the more opinionated approach that I want to be push on our own rules. Let me know if that makes sense:

Here are examples from our codebase (tweaked to be more generic) that I would like to make possible while enforcing line breaks for everything else that is multiline.

# good (must haves in my opinion)

# Last argument of a method call without parentheses

  # Rails controller before actin
  before_action :load_something, only [
    :show,
    :list,
    :do_some_update_logic,
    // ...
  ]

  # Rails model validation
  validate :name, presence: true, on: [:create, :activate], if: -> {
    active? && onboarded?
  }

  # same for model hooks, with some Sorbet binding
  after_commit :geolocate, unless: -> {
    T.bind(self, Address)
    archived? || invalid?
  }

# good (More "nice to haves" in my opinion)

# last argument of a method call with parentheses

  # Logging
  write_log(:error,  {
     "scheduler" => job.class.name,
    "shop_id" => shop.id,
    "message" => "Something wrong happened here",
  })

  # Monorail logging
  Monorail.produce("some_monorail_event/1.0",  {
    shop_id: shop_id,
    reason: "the real reason",
    reason_code: reason_code,
    // ...
  })

# single argument method call

  # T.let from Sorbet, on a large hash
  SOME_CONSTANT = T.let({
    label: "value",
    label2: "value2",
    label3: "value3",
    label4: "value4",
    label5: "value5",
  }, T::Hash[Symbol, String])
  
  # Simple array of a single hash (I like this one as it makes it obvious that the array is single element)
  errors = [{
    error: "Something went wrong",
    error_code: error_code,
    // ...
  }]

I think that summarizes what we really need. I added all the rules here focusing on consistency, but I'd hate myself if we just merged those "because Shopify needs them" when what we need is really a subset of it.

I think the extreme opposite of this PR, would be a really opinionated set of rules that:

1. Allows last parameter to be multiline for MultilineMethodArgumentLineBreaks and FirstMethodArgumentLineBreak when there are no parentheses (by default, or as an EnforcedStyle)
2. Same as above, but for all method calls.
3. Add an EnforcedStyle to Layout/FirstArrayElementLineBreak that allows single value arrays to be multiline.
4. Alternatively, do the above, to only allow single argument method calls to be multiline without applying MultilineMethodArgumentLineBreaks

[Korri]

@bbatsov with that additional context, do you think a more opinionated approach is worth pursuing, or should I clean up the doc and ship this approach instead?

[bbatsov]

@bbatsov with that additional context, do you think a more opinionated approach is worth pursuing, or should I clean up the doc and ship this approach instead?

I'm okay with the currently suggested approach, as I don't want to add too much complexity for this. We'll just need good examples in the docs (similar to what you've provided about the real-world use-cases), so that people can understand why such options exist in the first place.

[Korri]

@bbatsov I tweaked CopsDocumentationGenerator to allow for the changes I had done manually before and updated the example to include some of the real world examples I shared above.

I also confirmed that the changes work as expected (including the xref links) by generating the doc locally 👍

[Korri]

@bbatsov would love your 👁️ on this! Just rebased on master. Does someone with the right credentials need to run CircleCI Pipeline though? It keeps failing instantly.

[bbatsov]

Likely something went bad with the deploy key for CircleCI again... I've regened it just now, so you can try triggering a new build.

[Korri]

Likely something went bad with the deploy key for CircleCI again... I've regened it just now, so you can try triggering a new build.

Still failing :(

[Korri]

@bbatsov not sure why this is still failing, but I think the PR is otherwise ready for a (hopefully) final review.

[bbatsov]

Did you rebase this on top of master? I see that newer builds seem to be working normally.

[Korri]

Did you rebase this on top of master? I see that newer builds seem to be working normally.

@bbatsov yeah that's what the last 2 or 3 force-push are. Maybe it's because the branch is old?

EDIT: Nope failing on a different branch too

[Korri]

@bbatsov it's probably something related to my account and/or access right on Rubocop, as even an empty pull request (just an empty commit on top of master) fails for me: #11272

[bbatsov]

A recent PR changed this to return string.

[Korri]

I see yeah, you mean this commit right, either way it has no impact here, as we always end up using the department in the context of department_to_basename(department)

[Korri]

@bbatsov I did a rebase on master, and re-ran bundle exec rake update_cops_documentation just in case, there are no errors nor differences in the generated files 👍

[bbatsov]

Time to fly! 🚀