Style: Discuss how we want to format methods with very long argument/parameter lists

[elskwid]

Background

While working on #144 I had to add two more arguments to an already very long list. I found it more challenging than it needed to be for a few reasons:

1. The list expands horizontally so it's difficult to see if I may be adding duplicates
2. Keyword arguments aren't in a specific order so I spend time wondering where they should go
3. RuboCop could help enforce the line length (at least) but we have that disabled

Example

def in_span(name, attributes: nil, links: nil, start_timestamp: nil, kind: nil, sampling_hint: nil, with_parent: nil, with_parent_context: nil)
  span = start_span(name, attributes: attributes, links: links, start_timestamp: start_timestamp, kind: kind, sampling_hint: sampling_hint, with_parent: with_parent, with_parent_context: with_parent_context)
  with_span(span) { |s| yield s }
ensure
  span.finish
end

Proposal(s)

In order of the issues I encountered, here are my suggestions:

1. The list expands horizontally so it's difficult to see the entire list

Favor expanding code vertically. I find it much easier to visually parse a list of items vertically and our editors (and the devices we use to interact with them) are quite good at scrolling.

2. Keyword arguments aren't in a specific order so I spend time wondering where they should go and worrying that I may be adding duplicates

Favor alphabetical order for keyword arguments. This makes it trivial to see if duplicates are present.

3. RuboCop could help enforce the line length (at least) but we have that disabled

I'm not sure we're ready to turn the line length👮on but we use it on all our other Ruby projects and I miss the robotic security blanket. (For the record, I really like setting it at 80 characters and learning to love the constraint.)

I would like to see us move from this:

def in_span(name, attributes: nil, links: nil, start_timestamp: nil, kind: nil, sampling_hint: nil, with_parent: nil, with_parent_context: nil)
  span = start_span(name, attributes: attributes, links: links, start_timestamp: start_timestamp, kind: kind, sampling_hint: sampling_hint, with_parent: with_parent, with_parent_context: with_parent_context)
  with_span(span) { |s| yield s }
ensure
  span.finish
end

to something like this:

def in_span(name,
            attributes: nil,
            kind: nil,
            links: nil,
            sampling_hint: nil,
            start_timestamp: nil,
            with_parent_context: nil,
            with_parent: nil)

  span = start_span(name,
                    attributes: attributes,
                    kind: kind,
                    links: links,
                    sampling_hint: sampling_hint,
                    start_timestamp: start_timestamp,
                    with_parent_context: with_parent_context,
                    with_parent: with_parent)

  with_span(span) { |s| yield s }
ensure
  span.finish
end

The difference is even starker in a screenshot:

Or, perhaps this is more our speed 😄

def in_span(
  name,
  attributes: nil,
  kind: nil,
  links: nil,
  sampling_hint: nil,
  start_timestamp: nil,
  with_parent_context: nil,
  with_parent: nil
)

  span = start_span(
    name,
    attributes: attributes,
    kind: kind,
    links: links,
    sampling_hint: sampling_hint,
    start_timestamp: start_timestamp,
    with_parent_context: with_parent_context,
    with_parent: with_parent
  )

  with_span(span) { |s| yield s }
ensure
  span.finish
end

Or, my personal choice:

 def in_span(name,
             attributes: nil,
             kind: nil,
             links: nil,
             sampling_hint: nil,
             start_timestamp: nil,
             with_parent_context: nil,
             with_parent: nil)

  span = start_span(
    name,
    attributes: attributes,
    kind: kind,
    links: links,
    sampling_hint: sampling_hint,
    start_timestamp: start_timestamp,
    with_parent_context: with_parent_context,
    with_parent: with_parent
  )

  with_span(span) { |s| yield s }
ensure
  span.finish
end

Both examples pass RuboCop (as configured) and the test suite.

On indentation

I know people can get very passionate about indentation in these scenarios. I suggest we find something we can all agree on, make a pass through the code, and then stick to it even it's a mix of both 🙃.

[fbogsany]

Favor expanding code vertically. I find it much easier to visually parse a list of items vertically and our editors (and the devices we use to interact with them) are quite good at scrolling.

👍

Favor alphabetical order for keyword arguments. This makes it trivial to see if duplicates are present.

👍 although I tend to order keyword arguments (with defaults) by how frequently I specify them, but alphabetical order makes sense for very long argument lists.

I'm not sure we're ready to turn the line length👮on but we use it on all our other Ruby projects and I miss the robotic security blanket. (For the record, I really like setting it at 80 characters and learning to love the constraint.)

👎 I really find the constraint annoying - I already find myself fiddling with comment formatting to fit within 80 characters, and it really doesn't feel like the best use of my time. The constraint ends up forcing 💩 formatting in more cases than I'd like, and I end up adding rubocop:disable to sidestep the constraint, which lengthens the line.

[github-actions[bot]]

👋 This issue has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this issue will be closed eventually by the stale bot.