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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion rebar.config
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
]}
]}.

{plugins, [pc]}.
{plugins, [pc, rebar3_format]}.

% Interrupt compilation, if the artifact is not found
{artifacts, ["priv/exml_nif.so"]}.
Expand Down Expand Up @@ -50,3 +50,8 @@
{cover_export_enabled, true}.
{coveralls_coverdata, "_build/test/cover/eunit.coverdata"}.
{coveralls_service_name, "travis-ci"}.

{format, [
{formatter, default_formatter},
{options, #{sub_indent => 4}}
]}.
55 changes: 24 additions & 31 deletions src/exml.erl
Original file line number Diff line number Diff line change
Expand Up @@ -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.

to_binary/1,
to_iolist/1,
xml_size/1,
xml_sort/1,
to_pretty_iolist/1]).

-export([]).

-export_type([attr/0,
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


-type attr() :: {binary(), binary()}.
-type cdata() :: #xmlcdata{}.
Expand All @@ -39,23 +34,23 @@ xml_size([]) ->
0;
xml_size([Elem | Rest]) ->
xml_size(Elem) + xml_size(Rest);
xml_size(#xmlcdata{ content = Content }) ->
xml_size(#xmlcdata{content = Content}) ->
iolist_size(exml_nif:escape_cdata(Content));
xml_size(#xmlel{ name = Name, attrs = Attrs, children = [] }) ->
xml_size(#xmlel{name = Name, attrs = Attrs, children = []}) ->
3 % Self-closing: </>
+ byte_size(Name) + xml_size(Attrs);
xml_size(#xmlel{ name = Name, attrs = Attrs, children = Children }) ->
+ byte_size(Name)
+ xml_size(Attrs);
xml_size(#xmlel{name = Name, attrs = Attrs, children = Children}) ->
% Opening and closing: <></>
5 + byte_size(Name)*2
+ xml_size(Attrs) + xml_size(Children);
xml_size(#xmlstreamstart{ name = Name, attrs = Attrs }) ->
5 + byte_size(Name) * 2 + xml_size(Attrs) + xml_size(Children);
xml_size(#xmlstreamstart{name = Name, attrs = Attrs}) ->
byte_size(Name) + 2 + xml_size(Attrs);
xml_size(#xmlstreamend{ name = Name }) ->
xml_size(#xmlstreamend{name = Name}) ->
byte_size(Name) + 3;
xml_size({Key, Value}) ->
byte_size(Key)
+ 4 % ="" and whitespace before
+ 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.


%% @doc Sort a (list of) `xmlel()`.
%%
Expand All @@ -71,17 +66,14 @@ xml_size({Key, Value}) ->
xml_sort(#xmlcdata{} = Cdata) ->
Cdata;
xml_sort(#xmlel{} = El) ->
#xmlel{ attrs = Attrs, children = Children } = El,
El#xmlel{
attrs = lists:sort(Attrs),
children = [ xml_sort(C) || C <- Children ]
};
xml_sort(#xmlstreamstart{ attrs = Attrs } = StreamStart) ->
StreamStart#xmlstreamstart{ attrs = lists:sort(Attrs) };
#xmlel{attrs = Attrs, children = Children} = El,
El#xmlel{attrs = lists:sort(Attrs), children = [xml_sort(C) || C <- Children]};
xml_sort(#xmlstreamstart{attrs = Attrs} = StreamStart) ->
StreamStart#xmlstreamstart{attrs = lists:sort(Attrs)};
xml_sort(#xmlstreamend{} = StreamEnd) ->
StreamEnd;
xml_sort(Elements) when is_list(Elements) ->
lists:sort([ xml_sort(E) || E <- Elements ]).
lists:sort([xml_sort(E) || E <- Elements]).

-spec to_list(element() | [exml_stream:element()]) -> string().
to_list(Element) ->
Expand Down Expand Up @@ -112,10 +104,8 @@ to_iolist([_ | _] = Elements, Pretty) ->
Head = hd(Elements),
[Last | RevChildren] = lists:reverse(tl(Elements)),
case {Head, Last} of
{#xmlstreamstart{name = Name, attrs = Attrs},
#xmlstreamend{name = Name}} ->
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.

to_binary_nif(Element, Pretty);
_ ->
[to_iolist(El, Pretty) || El <- Elements]
Expand All @@ -133,6 +123,9 @@ to_iolist(#xmlcdata{content = Content}, _Pretty) ->
-spec to_binary_nif(element(), pretty | term()) -> binary().
to_binary_nif(#xmlel{} = Element, Pretty) ->
case catch exml_nif:to_binary(Element, Pretty) of
{'EXIT', Reason} -> erlang:error({badxml, Element, Reason});
Result when is_binary(Result) -> Result
{'EXIT', Reason} ->
erlang:error({badxml, Element, Reason});
Result when is_binary(Result) ->
Result
end.

25 changes: 15 additions & 10 deletions src/exml_nif.erl
Original file line number Diff line number Diff line change
Expand Up @@ -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


-export_type([parser/0, stream_element/0]).

-on_load(load/0).
-on_load({load, 0}).
erszcz marked this conversation as resolved.
Show resolved Hide resolved

%%%===================================================================
%%% Public API
Expand All @@ -33,29 +33,34 @@ load() ->
end,
erlang:load_nif(filename:join(PrivDir, ?MODULE_STRING), none).

-spec create(MaxChildSize :: non_neg_integer(), InfiniteStream :: boolean()) ->
{ok, parser()} | {error, Reason :: any()}.
-spec create(MaxChildSize :: non_neg_integer(), InfiniteStream :: boolean()) -> {ok,
parser()} |
{error,
Reason :: any()}.
create(_, _) ->
erlang:nif_error(not_loaded).

-spec escape_cdata(Bin :: iodata()) -> binary().
escape_cdata(_Bin) ->
erlang:nif_error(not_loaded).
erlang:nif_error(not_loaded).

-spec to_binary(Elem :: exml:element(), pretty | not_pretty) -> binary().
to_binary(_Elem, _Pretty) ->
erlang:nif_error(not_loaded).

-spec parse(Bin :: binary() | [binary()]) -> {ok, exml:element()} | {error, Reason :: any()}.
-spec parse(Bin :: binary() | [binary()]) -> {ok, exml:element()} |
{error, Reason :: any()}.
parse(_) ->
erlang:nif_error(not_loaded).

-spec parse_next(parser(), Data :: binary() | [binary()]) ->
{ok, stream_element() | undefined, non_neg_integer()} |
{error, Reason :: any()}.
-spec parse_next(parser(), Data :: binary() | [binary()]) -> {ok,
stream_element() | undefined,
non_neg_integer()} |
{error, Reason :: any()}.
parse_next(_, _) ->
erlang:nif_error(not_loaded).

-spec reset_parser(parser()) -> any().
reset_parser(_) ->
erlang:nif_error(not_loaded).

111 changes: 65 additions & 46 deletions src/exml_query.erl
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,24 @@
-export([cdata/1]).

-type element_with_ns() :: {element_with_ns, binary()}.
-type element_with_name_and_ns () :: {element_with_ns, binary(), binary()}.
-type element_with_attr_of_value () :: {element_with_attr, binary(), binary()}.
-type element_with_name_and_ns() :: {element_with_ns, binary(), binary()}.
-type element_with_attr_of_value() :: {element_with_attr, binary(), binary()}.
-type path() :: [cdata |
{element, binary()} |
{attr, binary()} |
element_with_ns() |
element_with_name_and_ns() |
element_with_attr_of_value()].

-type path() :: [cdata | %% selects cdata from the element
{element, binary()} | % selects subelement with given name
{attr, binary()} | % selects attr of given name
element_with_ns() | % selects subelement with given namespace
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 ;)

% 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
Comment on lines +33 to +41
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 :)


-export_type([path/0]).

Expand All @@ -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.

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

path(Child, Rest, Default);
path(#xmlel{} = Element, [{element_with_ns, NS} | Rest], Default) ->
Child = subelement_with_ns(Element, NS),
Expand Down Expand Up @@ -118,17 +127,20 @@ child_with_ns([#xmlel{} = Element | Rest], NS, Default) ->
child_with_ns([_ | Rest], NS, Default) ->
child_with_ns(Rest, NS, Default).

-spec subelement_with_attr(exml:element(), AttrName :: binary(), AttrValue :: binary()) ->
exml:element() | undefined.
-spec subelement_with_attr(exml:element(),
AttrName :: binary(),
AttrValue :: binary()) -> exml:element() | undefined.
subelement_with_attr(Element, AttrName, AttrValue) ->
subelement_with_attr(Element, AttrName, AttrValue, undefined).

-spec subelement_with_attr(Element, AttrName, AttrValue, Other) -> SubElement | Other when
Element :: exml:element(),
AttrName :: binary(),
AttrValue :: binary(),
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 :)

erszcz marked this conversation as resolved.
Show resolved Hide resolved
exml:element(),
AttrName :: binary(),
AttrValue :: binary(),
SubElement ::
exml:element(),
Other :: term().
subelement_with_attr(#xmlel{children = Children}, AttrName, AttrValue, Default) ->
child_with_attr(Children, AttrName, AttrValue, Default).

Expand All @@ -144,14 +156,15 @@ child_with_attr([#xmlel{} = Element | Rest], AttrName, AttrVal, Default) ->
child_with_attr([_ | Rest], AttrName, AttrVal, Default) ->
child_with_attr(Rest, AttrName, AttrVal, Default).


-spec subelement_with_name_and_ns(exml:element(), binary(), binary()) ->
exml:element() | undefined.
-spec subelement_with_name_and_ns(exml:element(), binary(), binary()) -> exml:element() |
undefined.
subelement_with_name_and_ns(Element, Name, NS) ->
subelement_with_name_and_ns(Element, Name, NS, undefined).

-spec subelement_with_name_and_ns(exml:element(), binary(), binary(), Other) ->
exml:element() | Other.
-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

subelement_with_name_and_ns(Element, Name, NS, Default) ->
case subelement(Element, Name, undefined) of
undefined ->
Expand All @@ -170,36 +183,41 @@ subelement_or_default(SubElement, NS, Default) ->

-spec subelements(exml:element(), binary()) -> [exml:element()].
subelements(#xmlel{children = Children}, Name) ->
lists:filter(fun(#xmlel{name = N}) when N =:= Name ->
true;
(_) ->
false
end, Children).
lists:filter(fun (#xmlel{name = N}) when N =:= Name ->
true;
(_) ->
false
end,
Children).

-spec subelements_with_ns(exml:element(), binary()) -> [exml:element()].
subelements_with_ns(#xmlel{children = Children}, NS) ->
lists:filter(fun(#xmlel{} = Child) ->
NS =:= attr(Child, <<"xmlns">>);
(_) ->
false
end, Children).

-spec subelements_with_name_and_ns(exml:element(), binary(), binary()) -> [exml:element()].
subelements_with_name_and_ns(#xmlel{children = Children}, Name, NS) ->
lists:filter(fun(#xmlel{name = SubName} = Child) ->
SubName =:= Name andalso
lists:filter(fun (#xmlel{} = Child) ->
NS =:= attr(Child, <<"xmlns">>);
(_) ->
false
end, Children).
(_) ->
false
end,
Children).

-spec subelements_with_name_and_ns(exml:element(),
binary(),
binary()) -> [exml:element()].
subelements_with_name_and_ns(#xmlel{children = Children}, Name, NS) ->
lists:filter(fun (#xmlel{name = SubName} = Child) ->
SubName =:= Name andalso NS =:= attr(Child, <<"xmlns">>);
(_) ->
false
end,
Children).

-spec subelements_with_attr(exml:element(), binary(), binary()) -> [exml:element()].
subelements_with_attr(#xmlel{children = Children}, AttrName, Value) ->
lists:filter(fun(#xmlel{} = Child) ->
Value =:= attr(Child, AttrName);
(_) ->
false
end, Children).
lists:filter(fun (#xmlel{} = Child) ->
Value =:= attr(Child, AttrName);
(_) ->
false
end,
Children).

-spec cdata(exml:element()) -> binary().
cdata(#xmlel{children = Children}) ->
Expand All @@ -217,3 +235,4 @@ attr(#xmlel{attrs = Attrs}, Name, Default) ->
false ->
Default
end.

24 changes: 7 additions & 17 deletions src/exml_stream.erl
Original file line number Diff line number Diff line change
Expand Up @@ -10,22 +10,11 @@

-include("exml_stream.hrl").

-export([new_parser/0,
new_parser/1,
parse/2,
reset_parser/1,
free_parser/1]).
-export([new_parser/0, new_parser/1, parse/2, reset_parser/1, free_parser/1]).

-export_type([element/0,
start/0,
stop/0,
parser/0,
parser_opt/0]).
-export_type([element/0, start/0, stop/0, parser/0, parser_opt/0]).

-record(parser, {
event_parser :: exml_nif:parser(),
buffer :: [binary()]
}).
-record(parser, {event_parser :: exml_nif:parser(), buffer :: [binary()]}).

-type start() :: #xmlstreamstart{}.
-type stop() :: #xmlstreamend{}.
Expand All @@ -46,7 +35,7 @@ new_parser() ->
new_parser([]).

-spec new_parser([parser_opt()]) -> {ok, parser()} | {error, any()}.
new_parser(Opts)->
new_parser(Opts) ->
MaxChildSize = proplists:get_value(max_child_size, Opts, 0),
InfiniteStream = proplists:get_value(infinite_stream, Opts, false),
case exml_nif:create(MaxChildSize, InfiniteStream) of
Expand All @@ -56,8 +45,8 @@ new_parser(Opts)->
Error
end.

-spec parse(parser(), binary()) -> {ok, parser(), [exml_stream:element()]}
| {error, Reason :: any()}.
-spec parse(parser(), binary()) -> {ok, parser(), [exml_stream:element()]} |
{error, Reason :: any()}.
parse(Parser, Input) when is_binary(Input) ->
#parser{event_parser = EventParser, buffer = OldBuf} = Parser,
Buffer = OldBuf ++ [Input],
Expand Down Expand Up @@ -101,3 +90,4 @@ drop_offset([Front | Rest], Offset) when byte_size(Front) =< Offset ->
drop_offset([Front | Rest], Offset) ->
<<_:Offset/binary, Part/binary>> = Front,
[Part | Rest].