-
Notifications
You must be signed in to change notification settings - Fork 22
Format code with rebar3_format plugin #52
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,20 +14,15 @@ | |
-include("exml_stream.hrl"). | ||
|
||
-export([parse/1]). | ||
|
||
-export([to_list/1, | ||
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]). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you using There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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{}. | ||
|
@@ -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). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why here the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()`. | ||
%% | ||
|
@@ -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) -> | ||
|
@@ -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)}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah… that's exactly the intention of |
||
to_binary_nif(Element, Pretty); | ||
_ -> | ||
[to_iolist(El, Pretty) || El <- Elements] | ||
|
@@ -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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you using There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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). | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
-export_type([path/0]). | ||
|
||
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one is very strange. I checked out There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 |
||
Name), % may return undefined | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is odd, indeed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have the default vaules |
||
path(Child, Rest, Default); | ||
path(#xmlel{} = Element, [{element_with_ns, NS} | Rest], Default) -> | ||
Child = subelement_with_ns(Element, NS), | ||
|
@@ -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 :: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to have the part in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). | ||
|
||
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, the formatter could even be smart enough to rewrite:
to:
For specs with more than, for example, 3 parameters. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 -> | ||
|
@@ -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}) -> | ||
|
@@ -217,3 +235,4 @@ attr(#xmlel{attrs = Attrs}, Name, Default) -> | |
false -> | ||
Default | ||
end. | ||
|
There was a problem hiding this comment.
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.