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

Add select methods returning element streams #2092

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Isira-Seneviratne
Copy link
Contributor

No description provided.

Copy link
Owner

@jhy jhy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks cool, thanks! Can you see the inline comments.

And, can you add tests. Would be good to create a testcase that compares some queries with the result of a select(query).stream().

src/main/java/org/jsoup/nodes/Element.java Outdated Show resolved Hide resolved
src/main/java/org/jsoup/nodes/Element.java Outdated Show resolved Hide resolved
src/main/java/org/jsoup/nodes/Element.java Outdated Show resolved Hide resolved
@jhy
Copy link
Owner

jhy commented Jul 30, 2024

I liked the original PR but I don't think I'm a fan of the changes in e1e35bb - all the getElementsStream methods. Generally the getElementBy... methods are only there to help users get used to jsoup coming from APIs like the W3C DOM APIs. They are not as powerful as the select methods. And extending them for direct stream access feels like its adding a bit of clutter for little value.

I think the selectStream(query) and selectStream(evaluator) should cover the core use case of this.

Also I think the test is still pretty anemic as it only selects one element. Doesn't test conditions where there's more or less results, or the order, etc. Would be great if you could beef those up.

@Isira-Seneviratne
Copy link
Contributor Author

Isira-Seneviratne commented Aug 1, 2024

Fair enough. I'll undo the newer changes.

Also, I was planning to add more tests, should've marked this PR as draft.

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

Successfully merging this pull request may close these issues.

2 participants