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

Multiline maps and lists should start on the same line as assignment #29

Closed
whatyouhide opened this issue Jul 27, 2017 · 16 comments
Closed
Labels

Comments

@whatyouhide
Copy link

map = %{
  a: 1,
}

should be left alone and not turned into

map =
  %{
    a: 1,
  }

Same for lists.

@lpil
Copy link

lpil commented Jul 28, 2017

Is this

map = %{
  a: 1,
  b: 2,
}

preferred over this

map = %{a: 1,
        b: 2}

with regards to the closing brace? For some reason I thought that the latter is what is done in the compiler.

@whatyouhide
Copy link
Author

The first one is preferred in "modern" Elixir. @josevalim confirm?

@whatyouhide
Copy link
Author

Btw the compiler is not the best example of Elixir code out there stylistically speakin :)

@lpil
Copy link

lpil commented Jul 28, 2017

Doh, I've been using it as my reference! Thanks for the help

@josevalim
Copy link

So Elixir was used to do this:

%{foo: 1,
  bar: 2}

But now we have changed it to:

%{
  foo: 1,
  bar: 2
}

When we had the former, I would indent the code like this:

map =
  %{foo: 1,
    bar: 2}

but now that we change the map formatting, I find this one too verbose:

map =
  %{
    foo: 1,
    bar: 2
  }

so we should probably stick with:

map = %{
  foo: 1,
  bar: 2
}

Keep in mind that this only applies to key-value pairs. Function calls, lists and so on they always run until the end of the line:

list =
  [1, 2, 3, 4, 5, 6, 7, 8, 9,
   10, 11, 12]

fun_call =
  some_fun(foo, bar, baz,
          baz)

@lpil
Copy link

lpil commented Jul 28, 2017

Thanks Jose. :)

@josevalim
Copy link

@lpil you are welcome! and feel free to dispute the post above. That's how we have been doing things but they are definitely up to debate - as we just changed maps.

@lpil
Copy link

lpil commented Jul 28, 2017

I am a little surprised by the function args, I would have gone for one of these.

some_fun(foo,
         bar,
         baz,
         baz)
some_fun(
  foo,
  bar,
  baz
  baz
)

@josevalim
Copy link

@lpil the issue is that some functions take 6 or 8 arguments. Usually a big pattern matching on the first argument and then 5 or 6 following. If we do one per line, we have this:

def some_function(
  %{module: module, function: function},
  arg1,
  arg2,
  arg3,
  arg4
) do
  ...
end

And that is just way too much? What do you think?

So we decided to try to fill as much as possible on a single line - which is what happens with lists. We have added this ability to Elixir master just recently. More info here: elixir-lang/elixir@3b7bf4e

@lpil
Copy link

lpil commented Jul 28, 2017

Once things get large I prefer one item per line as it makes it much clearer where each argument/pattern begins.

As a minor addition it's also a bit easier to edit if you use a linewise editor such as vim or emacs :)

@josevalim
Copy link

@lpil yes, I can also see that. I guess we will only know for sure after we try it. Here is another example:

case some_function(
       foo,
       bar,
       baz
     ) do
  ...

or:

case some_function(foo, bar,
                   baz) do
  ...

@josevalim
Copy link

I guess the above also has something like:

case some_function(
  foo,
  bar,
  baz
) do
  ...

@lexmag
Copy link

lexmag commented Jul 28, 2017

I think I always use the following in function definitions and function calls (1.):

case some_function(foo, bar,
                   baz) do

def some_function(foo, bar,
                  baz) do

It looks like a different thing than map = %{ (2.) to me. 2. is about data structures, while the 1. is not.
Also I think 1. indentation works better if you have multiline pattern matching in arguments, and when conditions in definition.

@whatyouhide
Copy link
Author

This issue also applies to structs so I will reopen until solved for structs as well.

@whatyouhide whatyouhide reopened this Aug 1, 2017
@uohzxela
Copy link
Owner

uohzxela commented Aug 2, 2017

@whatyouhide, fix for structs: 1c863c7

Let me know if there's anything I've missed.

@whatyouhide
Copy link
Author

Looks good.

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

No branches or pull requests

5 participants