Add a macro for asserting if two xmlel's are equal #30

mentels · 2017-08-04T13:29:04Z

The assertion is meant to be used in tests.

It's still lacking:

better documentation (for functions; in README)
appropriate tests

gadgetci · 2017-08-04T13:29:17Z

src/exml_utils.erl

+
+-include_lib("exml/include/exml_stream.hrl").
+
+-type xmlstreamelement() :: #xmlel{} | #xmlstreamstart{} | #xmlstreamend{}.


According to Compiler:

Warning: type xmlstreamelement() is unused

michalwski · 2018-11-06T18:31:03Z

@mentels any chance for finishing this sooner or later?

mentels · 2018-11-07T07:37:47Z

Will have a look this week.

codecov-io · 2018-11-15T13:31:40Z

Codecov Report

Merging #30 into master will increase coverage by 0.62%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #30      +/-   ##
==========================================
+ Coverage   87.31%   87.94%   +0.62%     
==========================================
  Files           4        4              
  Lines         134      141       +7     
==========================================
+ Hits          117      124       +7     
  Misses         17       17

Impacted Files	Coverage Δ
src/exml.erl	`91.89% <100%> (+1.89%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80a4ecf...4c9f845. Read the comment docs.

mentels · 2018-11-15T14:19:32Z

@michalwski it's ready. @erszcz have a look if the credits are OK.

erszcz · 2018-11-15T15:40:24Z

@mentels More than OK. Thanks!

michalwski

Thanks for pushing this forward. The code looks good. Tests are also great with a room for a small improvement (details in comments to the code).

michalwski · 2018-11-16T08:31:56Z

src/exml.erl

+      children = lists:sort([ xml_sort(C) || C <- Children ])
+     };
+xml_sort(#xmlstreamstart{ attrs = Attrs } = StreamStart) ->
+    StreamStart#xmlstreamstart{ attrs = lists:sort(Attrs) };


Any chance to cover this function clause with tests? It's not covered at this moment.

michalwski · 2018-11-16T08:34:22Z

src/exml.erl

+xml_sort(Elements) when is_list(Elements) ->
+    lists:sort([ xml_sort(E) || E <- Elements ]);
+xml_sort(Unknown) ->
+    Unknown.


I'm wondering if we need that at all. According to the function spec everything is covered and we never get to this clause (as long as valid item() is passed).

I remember adding it for the sake of convenience, when I was figuring out how to return stuff from the Rust parser to the Erlang layer and I returned some rubbish just to test the mechanism. It might not be needed outside experimental code and even if it would, it's trivial to add again.

michalwski · 2018-11-16T08:34:39Z

src/exml.erl

+xml_sort(#xmlstreamstart{ attrs = Attrs } = StreamStart) ->
+    StreamStart#xmlstreamstart{ attrs = lists:sort(Attrs) };
+xml_sort(#xmlstreamend{} = StreamEnd) ->
+    StreamEnd;


It'd be good to cover this one with tests as well.

fenek · 2018-11-17T08:34:51Z

src/exml.erl

+    #xmlel{ attrs = Attrs, children = Children } = El,
+    El#xmlel{
+      attrs = lists:sort(Attrs),
+      children = lists:sort([ xml_sort(C) || C <- Children ])


XML is order sensitive so sorting children can cause false positives in macro. I know that it might be useful to not care about the order sometimes but I'd prefer this to be less implicit, if we're going to keep it like this.

Indeed, good catch. Attributes should be sorted, but children should not. The original did not sort here - https://github.com/erszcz/rxml/blob/e8483408663f0bc2af7896e786c1cdea2e86e43d/test/exml_test.hrl#L23

mentels · 2018-11-20T12:45:45Z

Yeah, I was thinking about it and it seemed to me that the children are order-independent too. But apparently they're not (I assume you're right). Will fix it. Thanks.

…

On Tue, Nov 20, 2018 at 1:23 PM Radosław Szymczyszyn < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/exml.erl <#30 (comment)>: > +%% Sorting is defined as calling `lists:sort/1` at: +%% * all the `xmlel's provided (if there is a list of them) AND +%% * all the `xmlel' elements' attributes recursively (the root and descendants) AND +%% * all the `xmlel' children recursively (the root and descendants). +%% The order is ascending. +%% +%% The implementation of this function is subtle modification of +%% erszcz/rxml@e848340 +-spec <erszcz/rxml@e8483408663f0bc2af7896e786c1cdea2e86e43d+-spec> xml_sort(item() | [item()]) -> item() | [item()]. +xml_sort(#xmlcdata{} = Cdata) -> + Cdata; +xml_sort(#xmlel{} = El) -> + #xmlel{ attrs = Attrs, children = Children } = El, + El#xmlel{ + attrs = lists:sort(Attrs), + children = lists:sort([ xml_sort(C) || C <- Children ]) Indeed, good catch. Attributes should be sorted, but children should not. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#30 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAtsQBTNMGTVfzGJTw-MIZotnD33kH1mks5uw_RWgaJpZM4Ott6r> .

-- Szymon Mentel M: +48 795 419 568 W: http://www.erlang-solutions.com E: szymon.mentel@erlang-solutions.com Code Sync and Erlang Solutions conferences: Code BEAM Lite <https://codesync.global/conferences/cbl-amsterdam-2018/> - Amsterdam - 30 November 2018 Code BEAM Lite <https://codesync.global/conferences/cbl-munich-2018/> - Munich - 7 December 2018 Lambda Days <http://www.lambdadays.org/lambdadays2019> - Kraków - 21-22 February 2019 Code BEAM SF <https://codesync.global/conferences/code-beam-sf-2019/> - San Francisco - 28 February - 1 March 2019 Code BEAM Lite <https://codesync.global/conferences/code-beam-lite-bologna/>- Bologna - 22 March 2019 ElixirConf EU <https://www.elixirconf.eu/> - Prague - 8-9 April 2019 Code BEAM STO <https://codesync.global/conferences/code-beam-sto-2019/> - Stockholm - 16-17 May 2019 Code Elixir LDN <https://codesync.global/conferences/code-elixir-ldn-2019/> - London - 18 July 2019 Code Mesh LDN - London - 8-9 November 2019 *Erlang Solutions and Code Sync care about your data and privacy; please find all details about the basis for communicating with you and the way we process your data in our Privacy Policy <https://www.erlang-solutions.com/privacy-policy.html> You can update your email preferences or opt-out from receiving marketing emails here <http://www2.erlang-solutions.com/emailpreference>.*

mentels · 2018-11-23T11:27:14Z

The PR is improved. Will squash the commits once accepted. Have a look @michalwski @erszcz @fenek

michalwski · 2018-11-23T12:33:10Z

test/exml_tests.erl

+sort_xmlstreamend_test() ->
+    SE1 = #xmlstreamend{name = <<"n1">>},
+    SE2 = SE1,
+    ?assertEqual(SE1, SE2).


@mentels Did you miss a call to exml:xml_sort on one of the arguments?

mentels · 2018-11-23T12:37:25Z

Ah, yeah, I did :)

…

On Fri, Nov 23, 2018 at 1:33 PM Michał Piotrowski ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/exml_tests.erl <#30 (comment)>: > @@ -71,7 +71,20 @@ sort_xmlel_nested_test() -> }, El2 = El1#xmlel{ children = lists:reverse(Children) }, ?assertNotEqual(El1, El2), - ?assertEqual(exml:xml_sort(El1), exml:xml_sort(El2)). + %% children order matters + ?assertNotEqual(exml:xml_sort(El1), exml:xml_sort(El2)). + +sort_xmlstreamstart_test() -> + Attrs = [{<<"attr1">>, <<"bar">>}, {<<"attr2">>, <<"baz">>}], + SS1 = #xmlstreamstart{name = <<"n1">>, attrs = Attrs}, + SS2 = SS1#xmlstreamstart{attrs = lists:reverse(Attrs)}, + ?assertNotEqual(SS1, SS2), + ?assertEqual(exml:xml_sort(SS1), exml:xml_sort(SS2)). + +sort_xmlstreamend_test() -> + SE1 = #xmlstreamend{name = <<"n1">>}, + SE2 = SE1, + ?assertEqual(SE1, SE2). @mentels <https://github.com/mentels> Did you miss a call to exml:xml_sort on one of the arguments? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#30 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAtsQEmUfz4WXktBBo9tpAE5hqIOAT8tks5ux-sIgaJpZM4Ott6r> .

-- Szymon Mentel M: +48 795 419 568 W: http://www.erlang-solutions.com E: szymon.mentel@erlang-solutions.com Code Sync and Erlang Solutions conferences: Code BEAM Lite <https://codesync.global/conferences/cbl-amsterdam-2018/> - Amsterdam - 30 November 2018 Code BEAM Lite <https://codesync.global/conferences/cbl-munich-2018/> - Munich - 7 December 2018 Lambda Days <http://www.lambdadays.org/lambdadays2019> - Kraków - 21-22 February 2019 Code BEAM SF <https://codesync.global/conferences/code-beam-sf-2019/> - San Francisco - 28 February - 1 March 2019 Code BEAM Lite <https://codesync.global/conferences/code-beam-lite-bologna/>- Bologna - 22 March 2019 ElixirConf EU <https://www.elixirconf.eu/> - Prague - 8-9 April 2019 Code BEAM STO <https://codesync.global/conferences/code-beam-sto-2019/> - Stockholm - 16-17 May 2019 Code Elixir LDN <https://codesync.global/conferences/code-elixir-ldn-2019/> - London - 18 July 2019 Code Mesh LDN - London - 8-9 November 2019 *Erlang Solutions and Code Sync care about your data and privacy; please find all details about the basis for communicating with you and the way we process your data in our Privacy Policy <https://www.erlang-solutions.com/privacy-policy.html> You can update your email preferences or opt-out from receiving marketing emails here <http://www2.erlang-solutions.com/emailpreference>.*

michalwski · 2018-11-23T13:37:30Z

Looks good to me now. Thanks!

fenek · 2018-11-26T15:50:07Z

@mentels Looks good. If you'd like to squash commits, then I think you may do so now. :)

The assertion is meant to be used in tests.

mentels · 2018-12-12T08:42:05Z

@fenek done.

gadgetci reviewed Aug 4, 2017

View reviewed changes

arcusfelis added the WIP Don't review WIP-s! label May 10, 2018

mentels force-pushed the exml-assertion branch from a17386d to 41912ce Compare November 15, 2018 11:25

mentels changed the title ~~[WIP] Add a macro for asserting if two xmlel's are equal~~ Add a macro for asserting if two xmlel's are equal Nov 15, 2018

mentels removed the WIP Don't review WIP-s! label Nov 15, 2018

michalwski suggested changes Nov 16, 2018

View reviewed changes

fenek requested changes Nov 17, 2018

View reviewed changes

michalwski reviewed Nov 23, 2018

View reviewed changes

michalwski approved these changes Nov 23, 2018

View reviewed changes

fenek approved these changes Nov 26, 2018

View reviewed changes

Add a macro for asserting if two xmlel's are equal

4c9f845

The assertion is meant to be used in tests.

mentels force-pushed the exml-assertion branch from 28b9bf4 to 4c9f845 Compare December 12, 2018 08:41

michalwski merged commit a307e83 into master Dec 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a macro for asserting if two xmlel's are equal #30

Add a macro for asserting if two xmlel's are equal #30

mentels commented Aug 4, 2017 •

edited

gadgetci Aug 4, 2017

michalwski commented Nov 6, 2018

mentels commented Nov 7, 2018

codecov-io commented Nov 15, 2018 •

edited

mentels commented Nov 15, 2018

erszcz commented Nov 15, 2018

michalwski left a comment

michalwski Nov 16, 2018

michalwski Nov 16, 2018

erszcz Nov 16, 2018

michalwski Nov 16, 2018

fenek Nov 17, 2018

erszcz Nov 20, 2018 •

edited

mentels commented Nov 20, 2018 via email

mentels commented Nov 23, 2018

michalwski Nov 23, 2018

mentels commented Nov 23, 2018 via email

michalwski commented Nov 23, 2018

fenek commented Nov 26, 2018

mentels commented Dec 12, 2018


		-include_lib("exml/include/exml_stream.hrl").

		-type xmlstreamelement() :: #xmlel{} \| #xmlstreamstart{} \| #xmlstreamend{}.

Add a macro for asserting if two xmlel's are equal #30

Add a macro for asserting if two xmlel's are equal #30

Conversation

mentels commented Aug 4, 2017 • edited

gadgetci Aug 4, 2017

Choose a reason for hiding this comment

michalwski commented Nov 6, 2018

mentels commented Nov 7, 2018

codecov-io commented Nov 15, 2018 • edited

Codecov Report

mentels commented Nov 15, 2018

erszcz commented Nov 15, 2018

michalwski left a comment

Choose a reason for hiding this comment

michalwski Nov 16, 2018

Choose a reason for hiding this comment

michalwski Nov 16, 2018

Choose a reason for hiding this comment

erszcz Nov 16, 2018

Choose a reason for hiding this comment

michalwski Nov 16, 2018

Choose a reason for hiding this comment

fenek Nov 17, 2018

Choose a reason for hiding this comment

erszcz Nov 20, 2018 • edited

Choose a reason for hiding this comment

mentels commented Nov 20, 2018 via email

mentels commented Nov 23, 2018

michalwski Nov 23, 2018

Choose a reason for hiding this comment

mentels commented Nov 23, 2018 via email

michalwski commented Nov 23, 2018

fenek commented Nov 26, 2018

mentels commented Dec 12, 2018

mentels commented Aug 4, 2017 •

edited

codecov-io commented Nov 15, 2018 •

edited

erszcz Nov 20, 2018 •

edited