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

traverse_and_update/3 return typespec needs html_tree() #426

Closed
hattmarris opened this issue Oct 4, 2022 · 4 comments · Fixed by #431
Closed

traverse_and_update/3 return typespec needs html_tree() #426

hattmarris opened this issue Oct 4, 2022 · 4 comments · Fixed by #431
Labels

Comments

@hattmarris
Copy link

Description

The first element in the tuple return type was changed tohtml_node() in this revision - https:/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}
@hattmarris hattmarris added the Bug label Oct 4, 2022
@philss
Copy link
Owner

philss commented Oct 4, 2022

@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 philss assigned philss and unassigned philss Oct 29, 2022
@philss
Copy link
Owner

philss commented Oct 29, 2022

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

@WLSF
Copy link
Contributor

WLSF commented Oct 30, 2022

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

philss commented Oct 31, 2022

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

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

Successfully merging a pull request may close this issue.

3 participants