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

Clarification on multiline defs #204

Open
vincent-lg opened this issue Jul 10, 2022 · 3 comments
Open

Clarification on multiline defs #204

vincent-lg opened this issue Jul 10, 2022 · 3 comments

Comments

@vincent-lg
Copy link

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:


  • Run single-line defs that match for the same function together, but separate
    multiline defs with a blank line.
    [link]

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

  • If you have more than one multiline def, do not use single-line defs.
    [link]

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

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
Copy link
Owner

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
Copy link

dotdotdotpaul commented Jul 11, 2022 via email

@christopheradams
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants