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

Format code with rebar3_format plugin #52

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

michalwski
Copy link

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.

Comment on lines +33 to +41
%% 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
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

@@ -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,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@michalwski michalwski Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔
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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@michalwski michalwski Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

+ byte_size(Value).
byte_size(Key) +
4 % ="" and whitespace before
+ byte_size(Value).
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because of the comment, indeed.

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)},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

cdata/0,
element/0,
item/0]).
-export_type([attr/0, cdata/0, element/0, item/0]).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@elbrujohalcon elbrujohalcon Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is here: AdRoll/rebar3_format#88

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

-export([parse/1]).

-export([to_list/1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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]).
Copy link
Member

@erszcz erszcz Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

@elbrujohalcon elbrujohalcon Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened the issue here: AdRoll/rebar3_format#88

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🤷‍♂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/exml_query.erl Show resolved Hide resolved
@michalwski
Copy link
Author

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

elbrujohalcon commented Mar 16, 2020

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

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

@erszcz
Copy link
Member

erszcz commented Mar 17, 2020

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

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

Successfully merging this pull request may close these issues.

3 participants