christopheradams / elixir_style_guide

A community driven style guide for Elixir
4.36k stars 300 forks source link

Clarification on multiline defs #204

Open vincent-lg opened 2 years ago

vincent-lg commented 2 years ago

Greetings,

First of all, thank you for the guide.

I have some difficulty understanding two points that seem (to me) to conflict. I'm not saying they're not exact, but I have trouble guessing what they mean, maybe others will too:

The way I see it, the second point seems to contradict the first one. No one-line clause whenever we have a clause of the same function spanning multiple lines? This arises a lot when dealing with recursion. Some clauses (the ones dealing with empty collections in particular) can be extremely short while some can be more verbose. As a teacher (and writer), I'd prefer to follow some consistency.

From what I've seen, the first point seems to be used by most Elixir developers: one-line clauses are grouped together but multiline clauses are isolated by a blank line. That's the convention I personally follow.

I probably misunderstood the second point. A clarification (on this issue or in the README itself) would be great.

Thank you again,

christopheradams commented 2 years ago

Given that the formatter enforces blank lines between multiline defs, the style conventions could be reworded as:

  1. Don't separate single-line defs of the same function with blank lines.
  2. If you have more than one multiline def of the same function, don't use single-line style at all.

The first rule seems like a sensible style suggestion. The second one I'm unsure about. I can imagine (like you suggest) a recursive function with a series of single-line defs, followed by two multi-line defs, followed by another series of single-line defs, etc. Re-writing them as all as multiline just to follow this style rule wouldn't seem sensible.

Does anyone have strong opinions about the multiple-function-defs rule?

See #39 and #116

dotdotdotpaul commented 2 years ago

A happy medium might be: If your function has multiple implementations, single-line implementations at the "top" should be single-spaced (ie. don't separate the lines), but after the first multi-line implementation, double-space (blank line between) all subsequent functions, and don't use single-line functions after. There are a few cases I can think of where this would be a pain in the butt, but few and far between. This probably comes up mostly with a catch-all function that "just returns an error" and would love to make that a single line function. But personally, my "style" would look like:

Batch the one-liners at the top

def foo(nil), do: nil def foo(), do: nil

Now space out for the multi-line functions

def foo() do ... end

def foo() do ... end

Would love to make this a one liner...

def foo(arg), do: {:error, "Invalid arg #{inspect(arg)}"}

But should be a multi-line to be consistent...

def foo(arg) do {:error, "Invalid arg #{inspect(arg)}"} end

I think mixing single- and multi-line implementations is okay, but limit the single-line implementations "to the top". "Once you go multi-, you never go back." :)

...Paul

On Mon, Jul 11, 2022 at 3:55 AM Christopher Adams @.***> wrote:

Given that the formatter enforces blank lines between multiline defs, the style conventions could be reworded as:

  1. Don't separate single-line defs of the same function with blank lines.
  2. If you have more than one multiline def of the same function, don't use single-line style at all.

The first rule seems like a sensible style suggestion. The second one I'm unsure about. I can imagine (like you suggest) a recursive function with a series of single-line defs, followed by two multi-line defs, followed by another series of single-line defs, etc. Re-writing them as all as multiline just to follow this style rule wouldn't seem sensible.

Does anyone have strong opinions about the multiple-function-defs https://github.com/christopheradams/elixir_style_guide#multiple-function-defs rule?

See #39 https://github.com/christopheradams/elixir_style_guide/pull/39 and #116 https://github.com/christopheradams/elixir_style_guide/issues/116

— Reply to this email directly, view it on GitHub https://github.com/christopheradams/elixir_style_guide/issues/204#issuecomment-1179908870, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAANTBKROWKKTRLVBPAFGJTVTOEKNANCNFSM53FONQIQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

christopheradams commented 1 year ago

Looking again at the multiple-function-defs rule, it was introduced in #39 in order to close #23, but the stated rationale of "not mixing single-line and multi-line for more than one multi-line def" was accepted without discussion.

I think we can agree on the rule to remove blank lines between single-line defs and add them between multi-line defs:

def some_function(nil), do: {:error, "No Value"}
-
def some_function([]), do: :ok
+
def some_function([first | rest]) do
  some_function(rest)
end

But if we were to add another multi-line def, do we really need to re-write everything to be multi-line? Any small benefit to readability I think is outweighed by the downsides of having to re-write perfectly fine single-line defs, which would complicate the commit diff (and any time you re-write something you could introduce bugs or regressions).

I would propose dropping the multiple-function-defs rule since it seems unnecessarily difficult to understand, use, and maintain the style.