mix format exceeds line length when pipeline is used as value of module attribute

[KronicDeth]

Environment

• Elixir & Erlang versions (elixir --version):

  
  Erlang/OTP 20 [erts-9.2] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:10] [hipe] [kernel-poll:false] [dtrace]

Elixir 1.6.1 (compiled with OTP 20)

* Operating system:  macOS 10.13.3

### Current behavior

When a module attribute's value is a pipeline and one of the lines of the pipeline exceeds the line limit, the entire pipeline is not moved to the next line

##### `.formatter.exs`

```elixir
[
  inputs: [".credo.exs", ".formatter.exs", "mix.exs", "{config,lib,priv,rel,test}/**/*.{ex,exs}"],
  line_length: 120,
  rename_deprecated_at: "1.6.0"
]

(truncated) notes_web/controllers/note_controller.ex

defmodule NotesWeb.NoteController do
  @pointer_path_from_ecto_changeset_error_field_options @ecto_schema_module
                                                        |> Alembic.Source.pointer_path_from_ecto_changeset_error_field_options_from_ecto_schema_module()
                                                        |> update_in(
                                                          [:association_set],
                                                          &MapSet.difference(&1, @embedded_attribute_set)
                                                        )
                                                        |> update_in(
                                                          [:attribute_set],
                                                          &MapSet.union(&1, @embedded_attribute_set)
                                                        )
                                                        |> Map.put(
                                                          :format_key,
                                                          &JaSerializer.Formatter.Utils.format_key/1
                                                        )

end

Expected behavior

The line length should not be exceeded.

There are two ways I can think of to make this work with module attribute value parsing

Use \ to continue line

Use \ to continue the line of the module attribute name and then do the 2 space indent of the pipeline like for =

defmodule NotesWeb.NoteController do
  @pointer_path_from_ecto_changeset_error_field_options \
    @ecto_schema_module
    |> Alembic.Source.pointer_path_from_ecto_changeset_error_field_options_from_ecto_schema_module()
    |> update_in([:association_set], &MapSet.difference(&1, @embedded_attribute_set))
    |> update_in([:attribute_set], &MapSet.union(&1, @embedded_attribute_set))
    |> Map.put(:format_key, &JaSerializer.Formatter.Utils.format_key/1)
end

Use ( to continue expression

Use ( ) to group the pipeline, so that the ( ensures the module attribute still sees its value

defmodule NotesWeb.NoteController do
  @pointer_path_from_ecto_changeset_error_field_options (
    @ecto_schema_module
    |> Alembic.Source.pointer_path_from_ecto_changeset_error_field_options_from_ecto_schema_module()
    |> update_in([:association_set], &MapSet.difference(&1, @embedded_attribute_set))
    |> update_in([:attribute_set], &MapSet.union(&1, @embedded_attribute_set))
    |> Map.put(:format_key, &JaSerializer.Formatter.Utils.format_key/1)
  )
end

Work-arounds

Either of the proposed expected behaviors work as a work-around because the formatter removes the \ or ( and goes back to the bugged format.

Breaking up the pipeline is all I could find to fix the problem. I also ended up using compile-time local variables as it would allow me to put the very long function name on a new line after the =.

defmodule NotesWeb.NoteController do
  embedded_attribute_set = MapSet.new(~w(author recipients)a)

  pointer_path_from_ecto_changeset_error_field_options =
    ecto_schema_module
    |> Alembic.Source.pointer_path_from_ecto_changeset_error_field_options_from_ecto_schema_module()
    |> update_in([:association_set], &MapSet.difference(&1, embedded_attribute_set))
    |> update_in([:attribute_set], &MapSet.union(&1, embedded_attribute_set))
    |> Map.put(:format_key, &JaSerializer.Formatter.Utils.format_key/1)

  @pointer_path_from_ecto_changeset_error_field_options pointer_path_from_ecto_changeset_error_field_options
end

[josevalim]

I think the only option for making this work that aligns well with the current formatter semantics is this:

@pointer_path_from_ecto_changeset_error_field_options(
  ...
)

Note no space between the attribute and the expression. The formatter typically keeps parens on function calls if the original input had them, so we should likely respect it here too.

Thoughts?

[KronicDeth]

Although I know from reimplementing the grammar that module attribute definitions are really calls, will that look weird to normal users that don't think of module attribute definitions as calls?

[josevalim]

That's a good point.

Another approach would be to respect parens generally but that's a very slippery slope. For example, the following would be fine with such rules:

def foo((foo), (bar), (baz)) do

Generalizing is always hard because it then allows constructs we didn't want to allow in the first place.

So I don't think we have a better option than leveraging the call construct.

[KronicDeth]

I guess this can be one of those things, like long strings, where if you see the formatter do it, it means you have to get manually involved (in the case of long strings doing the <> break up), so the call format will work and fix the bug, and if users don't like ( on module attributes, then they can do a rewrite like I had to do. It's my own fault for following a naming convention that ended up with such a long function name anyway. 😉

[josevalim]

@whatyouhide @lexmag I would like your opinion on this one. Should we have the option to keep the parens on module attributes?

[josevalim]

@KronicDeth given doing this change would allow folks to set attributes like @name(__MODULE__), I think we should wait a bit more before adding this flexibility. And, as you said, rearranging the code would be the best option in this particular case.

Thank you for the report! We may introduce this if we get to see more use cases.

[lexmag]

I'm personally okay with keeping parentheses as is for module attributes. However it will lead to extra style variations. Perhaps we could wait with taking that step and keep current behaviour (going over :line_length).

[whatyouhide]

I would also consider this code as code that inevitably goes over the line length (like a long string), so I am in favour of not using parens yet in order to avoid allowing more than one style here :).

On Tue, 27 Feb 2018 at 22:57, Aleksei Magusev notifications@github.com wrote:

I'm personally okay with keeping parentheses as is for module attributes. However it will lead to extra style variations. Perhaps we could wait with taking that step and keep current behaviour (going over :line_length).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/elixir-lang/elixir/issues/7379#issuecomment-369040746, or mute the thread https://github.com/notifications/unsubscribe-auth/ADtcSgWyLZZ37PDztmTtLf9LEimjIV9Xks5tZHotgaJpZM4SN5Uh .

--

Andrea Leopardi an.leopardi@gmail.com