-
Notifications
You must be signed in to change notification settings - Fork 15
Implement parent_nodes + nth_child #25
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
ad68952
f52b687
922e53b
f6a4964
a594dd9
50c0727
a98b1bc
83845cd
8669e1d
7e9d14e
0594c3f
e2dbf91
a4e7d69
1b73ce7
323deb7
9062d95
82de179
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 |
|---|---|---|
|
|
@@ -357,6 +357,60 @@ defmodule LazyHTML do | |
| LazyHTML.NIF.child_nodes(lazy_html) | ||
| end | ||
|
|
||
| @doc """ | ||
| Returns the (unique) parent nodes of the root nodes in `lazy_html`. | ||
|
|
||
| ## Examples | ||
|
|
||
| iex> lazy_html = LazyHTML.from_fragment(~S|<div><span>Hello</span> <span>world</span></div>|) | ||
| iex> spans = LazyHTML.query(lazy_html, "span") | ||
| iex> LazyHTML.parent_nodes(spans) | ||
| #LazyHTML< | ||
| 1 node (from selector) | ||
| #1 | ||
| <div><span>Hello</span> <span>world</span></div> | ||
| > | ||
|
|
||
| The root node is always <html>, even if initialized via `from_fragment/1`: | ||
|
|
||
| iex> lazy_html = LazyHTML.from_fragment(~S|<div>root</div>|) | ||
| iex> LazyHTML.parent_nodes(lazy_html) | ||
| #LazyHTML< | ||
| 1 node (from selector) | ||
| #1 | ||
| <html><div>root</div></html> | ||
| > | ||
|
Member
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 don't think that should be the case, for the end user we should make it such that the fragment root has no parent.
Contributor
Author
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 was more accidental given my c++ implementation. I'll check if I find out how to differentiate
Contributor
Author
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 looked into this, and I now think the current behavior is correct and preferable. The reason is that I could no longer write the If you still think I should change it, then we need to add a new function that allows identifying how a LazyHTML was constructed.
Contributor
Author
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. @jonatanklosko do you agree? If so I think this PR is ready and I'll remove the
Member
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 don't think it's correct if
Typically it's something for the API user to track, since they are the one calling either
Contributor
Author
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. Hmm, ok. I think I can work around it by checking if the last parent is an
Contributor
Author
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. @jonatanklosko I have changed the implementation in my latest commit. You were right, this is better. I couldn't find a way to identify if a document is a fragment or not in lexbor, so I tracked in manually at creation time. I think this is correct now, see the new tests.
Contributor
Author
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. @jonatanklosko Sorry for the ping. If just haven't gotten around to it yet, no worries, take your time. I believe this was the last open issue, so I'm waiting on your approval here. After that I'll remove the "nth-child selector" test and then I think this PR is ready for a final review. |
||
|
|
||
| """ | ||
| @spec parent_nodes(t()) :: t() | ||
| def parent_nodes(lazy_html) do | ||
|
Member
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 convention we follow is that the names are from perspective of a single node, so this should be
Contributor
Author
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. Ok, will do 馃憤 (plus remove the singular helpers)
Contributor
Author
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.
Wait, do you mean if I have multiple elements on the same level and I call parent_node(same_level_nodes) I should get back the same parent node n times?
Member
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.
Correct. In your use case it seems you target a specific element, so parent would always return either 1 or 0 elements. The reason is API consistency,
Contributor
Author
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. My mental modal for LazyHTML is a document + a set of selected nodes. Or alternatively, a document plus the result of a CSS selector. I would argue that a <div>
<span>1</span>
<span>2</span>
</div>I think that Also, what about getting siblings? I don't think this would be inconsistent with the API, other things that operate in a batch do return a list (apart from child_nodes).
Member
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. fragment = LazyHTML.from_fragment(~S"""
<div>
<div>1</div>
<div>2</div>
</div>
""")
fragment |> LazyHTML.query("div") |> LazyHTML.query("div")Currently this returns @josevalim do you have an opinion here, should we always return a set of nodes?
Member
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 am fine with treating it as a set, I assume such can be done cheaply?
Member
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. Whenever building new list (
Contributor
Author
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. Ohh. Yeah, that is definitely surprising to me. Especially since it already filters out empty results.. |
||
| LazyHTML.NIF.parent_nodes(lazy_html) | ||
| end | ||
|
|
||
| @doc """ | ||
| Returns the parent nodes of the root nodes in `lazy_html`. | ||
| Useful when you're expecting a single, shared parent. | ||
| """ | ||
| def parent_node(lazy_html) do | ||
|
spicychickensauce marked this conversation as resolved.
|
||
| parent = LazyHTML.NIF.parent_nodes(lazy_html) | ||
|
|
||
| case LazyHTML.NIF.num_nodes(parent) do | ||
| 0 -> {:ok, nil} | ||
| 1 -> {:ok, parent} | ||
| _ -> {:error, :multiple_parents} | ||
| end | ||
| end | ||
|
|
||
| @doc """ | ||
| Same as `parent_node/1` but raises on multiple parents | ||
| """ | ||
| def parent_node!(lazy_html) do | ||
| case parent_node(lazy_html) do | ||
| {:ok, res} -> res | ||
| {:error, :multiple_parents} -> raise "Selected nodes have multiple parents" | ||
| end | ||
| end | ||
|
|
||
| @doc """ | ||
| Returns the text content of all nodes in `lazy_html`. | ||
|
|
||
|
|
@@ -481,6 +535,29 @@ defmodule LazyHTML do | |
| LazyHTML.NIF.tag(lazy_html) | ||
| end | ||
|
|
||
| @doc """ | ||
| Returns true if the lazy_html is selecting the same nodes starting from the same document. | ||
|
|
||
| ## Examples | ||
|
|
||
| iex> lazy_html = LazyHTML.from_fragment(~S|<div><span id=1>Hello</span></div>|) | ||
| iex> a = LazyHTML.query(lazy_html, "#1") | ||
| iex> b = LazyHTML.query(lazy_html, "div > span") | ||
| iex> LazyHTML.equals?(a, b) | ||
| true | ||
|
|
||
| Note that if the lazy_htmls are created separately, they are never equal: | ||
|
|
||
| iex> html_a = LazyHTML.from_fragment(~S|<div>hello</div>|) | ||
| iex> html_b = LazyHTML.from_fragment(~S|<div>hello</div>|) | ||
| iex> LazyHTML.equals?(html_a, html_b) | ||
| false | ||
| """ | ||
| @spec equals?(t(), t()) :: boolean() | ||
| def equals?(html_a, html_b) do | ||
|
spicychickensauce marked this conversation as resolved.
Outdated
|
||
| LazyHTML.NIF.equals(html_a, html_b) | ||
| end | ||
|
|
||
| @doc ~S""" | ||
| Escapes the given string to make a valid HTML text. | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.