Format code with rebar3_format plugin #52

michalwski · 2020-03-16T13:41:10Z

Inspired by http://tech.nextroll.com/blog/dev/2020/02/25/erlang-rebar3-format.html I wanted to try the rebar3 format plugin with exml.

michalwski · 2020-03-16T14:17:47Z

src/exml_query.erl

+                                                %% selects cdata from the element
+                                                % selects subelement with given name
+
+                                    % selects attr of given name
+ % selects subelement with given namespace
+
+                                              % selects subelement with given name and namespace
+
+ % selects subelement with given attribute and value


Looks like the formatting put all the comments after the type specification. I wonder if anything can be done about it.

Check my other comment. It's a known issue, sadly. One that we tried to solve but couldn't.
If you feel like taking a deep dive into rebar3_format and submitting a PR for this, we would gladly accept it :)

michalwski · 2020-03-16T14:19:38Z

src/exml_query.erl

@@ -44,7 +52,8 @@ path(Element, Path) ->
 path(#xmlel{} = Element, [], _) ->
    Element;
 path(#xmlel{} = Element, [{element, Name} | Rest], Default) ->
-    Child = subelement(Element, Name), % may return undefined
+    Child = subelement(Element,


Why exactly this was split into two lines? I think the old line length is less that 100 chars.

This one is very strange. I checked out master branch of this repo on my computer, modified the rebar.config just like you did and run rebar3 format and it doesn't change the line.

I did the same @elbrujohalcon and the result is the same as before for me. Do you think it makes a difference what OS or Erlang version is used when running the formatter? I'm on newest Mac OS X and Erlang OTP 22.2

🤔
My Erlang is Erlang/OTP 21 [erts-10.3.5.3] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [hipe].
I can try with 22.2 later.

michalwski · 2020-03-16T14:21:13Z

src/exml_query.erl

-      SubElement :: exml:element(),
-      Other :: term().
+-spec subelement_with_attr(Element, AttrName, AttrValue, Other) -> SubElement |
+                                                                   Other when Element ::


Is it possible to have the part in when formatted starting from new line, as it was done before? Current rules makes formatting awkward in this case.

this is a great config option to add! Please open an issue in the rebar3_format repo :)

michalwski · 2020-03-16T14:23:22Z

src/exml_query.erl

+-spec subelement_with_name_and_ns(exml:element(),
+                                  binary(),
+                                  binary(),
+                                  Other) -> exml:element() | Other.


Looks like a rule moving the whole return spec to a new line would work better in case the line is too long. See the spec above also.

Actually, the formatter could even be smart enough to rewrite:

-spec subelement_with_name_and_ns(exml:element(), binary(), binary(), Other) -> exml:element() | Other.

to:

-spec subelement_with_name_and_ns(Element, Name, NS, Default) -> R when Element :: exml:element(), Name :: binary(), NS :: binary(), Default :: Other, R :: exml:element() | Other.

For specs with more than, for example, 3 parameters.

this is a great config option to add! Please open an issue in the rebar3_format repo :)

Here's one for spec expansion/rewriting - AdRoll/rebar3_format#90

michalwski · 2020-03-16T14:26:37Z

src/exml.erl

-    + byte_size(Value).
+    byte_size(Key) +
+        4 % ="" and whitespace before
+        + byte_size(Value).


Why here the + is at the begging while in the line it was moved to the line above? Is it because of the comment in the prev line?

It's because of the comment, indeed.

erszcz · 2020-03-16T14:18:50Z

src/exml.erl

-            Element = #xmlel{name = Name, attrs = Attrs,
-                             children = lists:reverse(RevChildren)},
+        {#xmlstreamstart{name = Name, attrs = Attrs}, #xmlstreamend{name = Name}} ->
+            Element = #xmlel{name = Name, attrs = Attrs, children = lists:reverse(RevChildren)},


A line that's wrapped by hand for record field readability is unwrapped automatically by the formatter.
Expected: One field per line should be preserved.

As I stated in another comment, the formatter doesn't care about how you formatted your code "by hand". It just reads AST and prints Erlang code. It will never preserve your style.

@elbrujohalcon how about making a rule (possibly configurable) telling the formatter to have one attribute of a record per line, starting with the first attribute in the same line as the record name? Sth like below:

Element = #xmlel{name = Name, attrs = Attrs, children = lists:reverse(RevChildren)},

Would that make sense?

Yeah… that's exactly the intention of inline_items. That's why it's weird that it's not working here.

erszcz · 2020-03-16T14:20:25Z

src/exml.erl

-              cdata/0,
-              element/0,
-              item/0]).
+-export_type([attr/0, cdata/0, element/0, item/0]).


All exported types are flattened onto one line. Expected: one type per line should be preserved.

Are you using inline_items => true in your options, by any chance?
If not, then this is a bug and you should report it in the rebar3_format repo.

@elbrujohalcon we use the default setting, which as far as I know is inline_items => {when_over, 25}

The issue is here: AdRoll/rebar3_format#88

erszcz · 2020-03-16T14:20:48Z

src/exml.erl

@@ -14,20 +14,15 @@
 -include("exml_stream.hrl").

 -export([parse/1]).
-
 -export([to_list/1,


This list of exports is left as-is. That's ok.

erszcz · 2020-03-16T14:21:49Z

src/exml_nif.erl

@@ -11,11 +11,11 @@
 -type parser() :: term().
 -type stream_element() :: exml:element() | exml_stream:start() | exml_stream:stop().

-export([create/2, parse/1, parse_next/2, escape_cdata/1,
-         to_binary/2, reset_parser/1]).
+-export([create/2, parse/1, parse_next/2, escape_cdata/1, to_binary/2, reset_parser/1]).


But this list of exports is flattened to one line (compare with https://github.com/esl/exml/pull/52/files#r393058908). Why? Is there a heuristic that if they're one function per line then they're left as is, but otherwise they're compacted to just one line?

Are you using inline_items => true in your options, by any chance?
If not, then this is a bug and you should report it in the rebar3_format repo.

I opened the issue here: AdRoll/rebar3_format#88

erszcz · 2020-03-16T14:22:26Z

src/exml_query.erl

-                 element_with_name_and_ns() | % selects subelement with given name and namespace
-                 element_with_attr_of_value() % selects subelement with given attribute and value
-                ].
+                                                %% selects cdata from the element


The comments are scattered around in a mess. Expected: comments left intact on their respective lines.

Yeah, this is a known issue. Since it's very hard to solve, we decided that (unless someone writes a PR solving it) for now the formatter can't deal with interleaved comments in type specs. The workaround is to put them all together above the type definition, like a single long comment block.
We know it's not ideal, but don't keep your hopes up for a solution. 🤷‍♂

In any case, if we rewrite this comment as follows, then it will also be picked up by EDoc and exported to the Docs chunk:

-type path() :: [cdata | {element, binary()} | {attr, binary()} | element_with_ns() | element_with_name_and_ns() | element_with_attr_of_value() ]. %% `path()' describes a relative position of a node in an XML document. The allowed path components are: %% - `cdata' - selects cdata from the element. %% - `{element, binary()}' - selects a subelement with a given name. %% - `{attr, binary()}' - selects an attribute of a given name. %% - `element_with_*' - select a subelement with given parameters.

The tools are there to help us, but sometimes we have to help the tools help us ;)

erszcz · 2020-03-16T14:23:16Z

src/exml_query.erl

@@ -44,7 +52,8 @@ path(Element, Path) ->
 path(#xmlel{} = Element, [], _) ->
    Element;
 path(#xmlel{} = Element, [{element, Name} | Rest], Default) ->
-    Child = subelement(Element, Name), % may return undefined
+    Child = subelement(Element,
+                       Name), % may return undefined


Why the line break? Sometimes the formatter compacts stuff onto one line, but in this case it inserted a line break due to no apparent reason.

This is odd, indeed.
Please report this as a bug on the rebar3_format repo.
On the other hand, have you tried with different values for paper and/or ribbon in your configuration? Maybe you have a too narrow paper configuration.

We have the default vaules paper => 100 and ribbon => 90

src/exml_query.erl

src/exml_nif.erl

michalwski · 2020-03-16T14:37:26Z

@elbrujohalcon we tried rebar3 format in this project and found some strange formatting issues. I hope this could be helpful for you in developing the tool and/or maybe you can tell us if at least some of the issues can be solved by configuration.

elbrujohalcon · 2020-03-16T14:46:26Z

@michalwski I replied to most comments.
The basic idea to wrap your head around here is that the formatter doesn't care about your current code style. It reads AST and prints Erlang code. So… it can't preserve any format you introduced "by hand" and that's by design.

elbrujohalcon · 2020-03-17T08:35:17Z

Please keep open bug reports and other issues on the rebar3_format repo folks. It's highly appreciated.

erszcz · 2020-03-17T08:45:08Z

Thanks for the review, @elbrujohalcon, and for pushing the formatter initiative forward!

michalwski added 2 commits March 16, 2020 14:39

Format code with rebar3_format plugin

b5467b0

Use 4 spaces for sub_ident

0b0cda3

michalwski commented Mar 16, 2020

View reviewed changes

erszcz reviewed Mar 16, 2020

View reviewed changes

src/exml_nif.erl Show resolved Hide resolved

michalwski mentioned this pull request Mar 16, 2020

List of exported functions and types are inlined despite the default {when_over, 25} setting AdRoll/rebar3_format#88

Closed

This was referenced Mar 17, 2020

Output -on_load(Fun/Arity) instead of -on_load({Fun, Arity}) AdRoll/rebar3_format#89

Closed

Formatting specs with 'when' keyword AdRoll/rebar3_format#90

Closed

Format code with rebar3_format plugin #52

Are you sure you want to change the base?

Format code with rebar3_format plugin #52

Conversation

michalwski commented Mar 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalwski Mar 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalwski Mar 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elbrujohalcon Mar 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erszcz Mar 16, 2020 • edited

Choose a reason for hiding this comment

elbrujohalcon Mar 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalwski commented Mar 16, 2020

elbrujohalcon commented Mar 16, 2020 • edited

elbrujohalcon commented Mar 17, 2020

erszcz commented Mar 17, 2020

michalwski Mar 17, 2020 •

edited

michalwski Mar 16, 2020 •

edited

elbrujohalcon Mar 16, 2020 •

edited

erszcz Mar 16, 2020 •

edited

elbrujohalcon Mar 16, 2020 •

edited

elbrujohalcon commented Mar 16, 2020 •

edited