Running the new Elixir formatter

syntax

#102

I rather quite prefer how the formatter is doing that. I always like the starting token to be at the end of it’s line, each element to be on their own, and the closing token to be on it’s own line at the same indentation as the indentation of the first line, just as it is doing. ^.^

I do, however, still wish for trailing comma’s… ^.^;


#103

It’s probably my time with Clojure showing- collapsed delimiters saves a lot of space and reduces noise when all you’re doing is writing nested data. It’s not as good for reorganizing, though.


#104

Yeah I do end-line closing tags in lisp as well, but that is because the ‘code’ is that way, for formatting data it is often good to see the bounds. ^.^


#105

Yeah, we only respect the user’s choice for data structures. We could also do it for function calls but function calls usually give us more liberty to handle those issues, for example by breaking into variables. And there is also the issue with function calls with no parens which have no way of respecting the user’s choice because there are no delimiters…


#106

Not really. The times I often use the multi-line function call syntax like that is when it is a macro and the AST is important. Look at ExSpirit for example (hmm, I need to test how elixir would format some grammars…). ^.^

EDIT1: Not very well, the grammar’s got a lot more right-heavy and the trailing comma’s all vanished, which is really important in grammars considering how much they are shuffled around while testing parsing… >.<

EDIT2: Also what on earth is it doing making this format?! o.O

    pipe_result_into: List.to_tuple()
    |> case do
         {:started, datetime_string} ->
           {:started, parse(datetime_string, parse_weirddatetimeformat()).result}

         result ->
           result
       end

EDIT3: It turned this:

  defrule parse_entry(seq([
    parse_type(),
    parse_body(),
  ])), pipe_result_into: List.to_tuple() |> (case do
    {:started, datetime_string} -> {:started, parse(datetime_string, parse_weirddatetimeformat()).result}
    result -> result
  end)

Into this:

  defrule(
    parse_entry(
      seq([
        parse_type(),
        parse_body()
      ])
    ),
    pipe_result_into: List.to_tuple()
    |> case do
         {:started, datetime_string} ->
           {:started, parse(datetime_string, parse_weirddatetimeformat()).result}

         result ->
           result
       end
  )

First of all, the function definition is all wrapped up in parenthesis, which is just wrong (that is like formatting def blah(x) to be def(blah(x))), and it formatted the pipe_result_into: option with the pipe into a ‘what the heck is this’, as well as a few other oddities (more minor ones like the seq call not being on the primary line and such).

The thing that bugs me most is how it is doing that piping in the pipe_result_into option, it makes me confused as to if it is piping the output of the List.to_tuple() call or if it is piping the output of the parse_entry call. I’m still having issues mentally parsing that even though I know it is fairly obvious… >.<


#107

Part of why it was so striking to me was a couple lines later another multi-line compare call with literally 2 more characters was not collapsed, and so now I have two visual representations of something that should stand out visually as (no pun intended) comparable.

This is how the formatter leaves this.

def eval(%Expr.Comparative{op: :!=} = expr, node, document, context) do
  v1 = Expr.eval(expr.e1, node, document, context)
  v2 = Expr.eval(expr.e2, node, document, context)
  compare(expr.op, Expr.Helpers.eq_fmt(v1, v2, document), Expr.Helpers.eq_fmt(v2, v1, document))
end

def eval(expr, node, document, context) do
  v1 = Expr.eval(expr.e1, node, document, context)
  v2 = Expr.eval(expr.e2, node, document, context)

  compare(
    expr.op,
    Expr.Helpers.cmp_fmt(v1, v2, document),
    Expr.Helpers.cmp_fmt(v2, v1, document)
  )
end

I guess one could argue there is a difference between the two versions of the function that it now better highlighted, but it seems off to me.

That’s true, but in this case I purposefully chose to use a multi-line function call rather than adding two more variables because I felt it produced the clearer code.


#108

There is an option for declaring locals_without_parens, it is in the docs.

For the keyword, we need to do something but I am not sure what. Probably treat it in the same way we would treat =. In this case it would mean breaking into the next line and indenting with two spaces. Please open up an issue.

Good point, please open up an issue too!


#109

I could define that in ex_spirit sure, but how would the users use it without each-and-every-single-one-of-them adding it to their options as well? It should be some in-file definition, like a module attribute or something to state ‘all uses of this is without parenthesis’ or so.

That would work but is more wordy in this case (more so in other cases as they tend to be really short, this example is longer than most).

Done. ^.^


#110

The above formatted to this without the parenthesis:

  defrule parse_entry(
            seq([
              parse_type(),
              parse_body()
            ])
          ),
          pipe_result_into: List.to_tuple()
          |> (case do
                {:started, datetime_string} ->
                  {:started, parse(datetime_string, parse_weirddatetimeformat()).result}

                result ->
                  result
              end)

However, why is it indenting it *so*far*to*the*right*? o.O?!
That action is making some of the deeper grammars very unreadable (especially as it is making the lines longer due to some other changes as well). Inside a (/) should be indentation, not alignment (absolutely should not be alignment).

So I can either get parenthesis around the rule function definition, or I get everything pushed waaaaaay over to the right. Neither of these options are right, not right at all… O.o!


#111

I’ve been playing with ExSpirit a lot. It’s a great library but it’s fundamentally weird. It does unconventional things to the AST. It’s a great DSL but it doesn’t mesh exactly well with the wider Elixir code. I dont’ think any effort should be expended in improving the output of the formatter on ExSpirit.
EDIT: ExSpirit is great for reasons other than the DSL, I don’t want to imply that the DSL is the only (or even major) reason you should pick ExSpirit!

Most of the time I actually put the seq in its own line, but as I’ve said above, I don’t think normal formatting rules apply to ExSpirit. Maybe ExSpirit need its own style guide? xD I don’t think you should ever expect good results out of running the formatter on ExSpirit.

I agree, this is weird. But again, see my point above.


#112

In the Javascript ecosystem (where I spend my professional life) we have a formatter called prettier. It’s used in quite a few popular codebases, albeit completely opinionated except for a few tiny options:

  • Print Width
  • Tab Width
  • Tabs or Spaces
  • Semicolons or no Semicolons
  • Single or Double Quotes
  • Trailing commas or no trailing commas
  • Bracket spacing {thing: true} vs { thing: true }

We make do with these options (I just converted several repos to prettier at my work) and no one really complains because we auto-format inside our CI pipelines.


#113

That’s what the discussion with @NobbZ above has been about. It can’t be anything set programmatically because the formatter does not evaluate code, it has no semantic understanding. And I would also be wary if we allowed libraries to implicitly tell how my code should be formatted, because they would be able to change how unrelated code to the library would be formatted too.

Parens are indented with two spaces since it has delimeters. Function calls without parens are indented by their name, that’s how most Elixir code is organized:

assert foo ==
       bar

If it wrote it as:

assert foo ==
  bar

Then some people would complain about the exact opposite that you are complaining. :slight_smile:


#114

The last two bugs reported have been fixed as well. Please give the formatter a try once more and let us know of more suspicious cases!


#115

Actually I’d vote for:

assert(
  foo == bar
)

If it managed to get longer that is. ^.^

I did, I saw the changes! I’ll grab master and try it again. :slight_smile:


#116

It now formats it to:

  defrule parse_entry(
            seq([
              parse_type(),
              parse_body()
            ])
          ),
          pipe_result_into:
            List.to_tuple()
            |> (case do
                  {:started, datetime_string} ->
                    {:started, parse(datetime_string, parse_weirddatetimeformat()).result}

                  result ->
                    result
                end)

Looks much more readable like that. :slight_smile:

Still far far too right (I’ve never seen the point to aligning to something above like that, only for repeated calls or so, which this is not the case, thus only indentation should be used, not alignment), and that random newline in the case is jarring, it either needs to be gone, or perhaps made into this (which I do in my especially large case statements, of which this is not one so I would not have empty lines at all):

  defrule parse_entry(
            seq([
              parse_type(),
              parse_body()
            ])
          ),
          pipe_result_into:
            List.to_tuple()
            |> (case do

                  {:started, datetime_string} ->
                    {:started, parse(datetime_string, parse_weirddatetimeformat()).result}

                  result ->
                    result

                end)

EDIT:
@josevalim https://github.com/elixir-lang/elixir/pull/6979 just went through, it formats:

alias Foo.{
        Bar
      }

As this instead now:

alias Foo.{
  Bar
}

Isn’t that the same thing that I want from this:

   defrule parse_entry(
            seq([
              parse_type(),
              parse_body()
            ])
          )

Wanting it to be this:

  defrule parse_entry(
    seq([
      parse_type(),
      parse_body()
    ])
  )

Seems the same kind of issue to me? And as such that PR becomes inconsistent considering the above case is not done too?
(Although I’d still put the trailing comma too as without it looks inconsistent and wrong).


#117

Prettier has saved me so much time in Javascript land, this makes me so excited for Elixir. It’s going to save brainpower, avoid hour long Slack discussions on “best practices” and pointless Wiki pages in source repositories.

:beers:


#118

That rule currently only applies to data-structures and if they are the last argument. Even if we generalized to function calls, it means it wouldn’t apply when you use :pipe_result_into, since the last argument is now the :pipe_result_into. Then you could say: “why not apply it to all arguments?”. The issue is that then you would get this:

defrule parse_entry(
  seq([
    parse_type(),
    parse_body()
  ])
),
pipe_result_into:
  List.to_tuple()
  |> (case do
        {:started, datetime_string} ->
          {:started, parse(datetime_string, parse_weirddatetimeformat()).result}

        result ->
          result
      end)

And that looks very confusing. And no, we can’t keep pipe_result_into in the previous line because once we break you get one argument per line.


#119

Actually what I’d prefer is this:

defrule parse_entry(
  seq([
    parse_type(),
    parse_body()
  ])
), pipe_result_into:
  List.to_tuple()
  |> (case do
        {:started, datetime_string} ->
          {:started, parse(datetime_string, parse_weirddatetimeformat()).result}
        result ->
          result
      end)

Specifically because they are just arguments to defrule. ^.^

Do note, defrule can have a do/end body as well, that body handles direct context manipulation. Which further makes it… hmm, interesting. ^.^;


#120

Ah hah! I just discovered why the indentation in the (/) and such look wrong to me in this formatter, it doesn’t match what the C++ formatting I use does (I’ve been doing a bit of C++ work recently)!

Specifically, in C++ if I have a function call where the arguments span over multiple lines it depends on if the first argument is on the same line as the ( or not, specifically (in elixir’eze):

# With the first argument *not* on the same line as the `(`:
bloop blah(
  arg0, # Normal 'indentation level', which should always be a <tab>, but elixir is weird with spaces (which is not indentation!)
  arg1,
  arg2 # I'd love a trailing comma here, C++ does not support it either though, blah...
)

# With the first argument *on* the same line as the `(`:
bloop blah(arg0,
           arg1, # indentation to the prior line (spaces in elixir), then spaces for alignment!  ^.^
           arg2
           ) # Sometimes I see this one character left so it is aligned with `(`, I don't really care either way though

And that is indeed what I was expecting before but was not getting it and did not know how to verbalize it at the time. :slight_smile:


#121

I just noticed this one.

I have a case where the name of something is represented by an atom, and one of the names is “true”, and so represented by the atom :true (I didn’t choose that name, it’s part of a specification).

You probably see where this is going, but:

def eval(%Expr.Function{f: :true, args: []}, _node, _document, _context) do

is formatted as

def eval(%Expr.Function{f: true, args: []}, _node, _document, _context) do

:true == true, but I’m trying to represent something called “true” as an atom, not use the boolean true.