philss/floki

traverse_and_update/3 return typespec needs html_tree()

hattmarris opened this issue · 4 comments

hattmarris commented

Description

The first element in the tuple return type was changed tohtml_node() in this revision - https://github.com/philss/floki/pull/289/files#diff-0eacaeb3de3bdf5b80408719a3ce69f47fd9eda5e9a02de33bd65f7b1235e0e2R395

But dialyzer caught for me that this was always returning an html_tree() spec'd as a list with a root node --
@type html_tree() :: [html_node()]

I don't pretend to know if maybe other use cases might return just a node - but if that's the case could we change spec for return type to allow html_tree() | html_node() node? Thanks in advance for taking the time to read my report & for the awesome lib!

To Reproduce

Steps to reproduce the behavior:

  • Using Floki v0.33.1
  • Using Elixir v1.14.0-otp-25
  • Using Erlang OTP v25.0.3
  • With this code:
    Here's a simple reproduction:
iex(27)> simple = "<div></div>"
"<div></div>"
iex(28)> Floki.parse_document!(simple) |> Floki.traverse_and_update(0, fn node, acc -> {node, acc} end)
{[{"div", [], []}], 0}

Expected behavior

Based on typespec, I expected:

- {[{"div", [], []}], 0}
+ {{"div", [], []}, 0}
philss commented

@hattmarris I think you are right. The return type should be {html_tree(), traverse_acc} as it was before. I don't know why I didn't catch that, but I think the correct is the previous version.

Would you mind to send a pull request?

philss commented

@WLSF I think I can't assign directly to you. But feel free to work on this :)

  • ❤️1
WLSF commented

@philss couple of things that I've noticed:

The function traverse_and_update/3 actually accepts a string as arg/response, as you can see on this test, is there a reason for it?

If thats by design then I guess that there are other aspects of this spec that might not be right, because right now its expecting the first arg and the response to be [html_node()], but as the test shows, its working with a simple string.

Assuming everything is ok with the STRING thing, I guess we could also replace this part by: html_node() since it already contains everything @type html_node :: html_tag() | html_comment() | html_doctype() | html_declaration() | html_text()

philss commented

@philss yeah, I was wrong. It should be html_node() | html_tree() as @hattmarris suggested.
And yeah, you could replace that part by html_node() 👍. Thanks for verifying it :)

  • ❤️1