-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
base: master
Are you sure you want to change the base?
Conversation
3d1e824
to
31d299a
Compare
There was a problem hiding this 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()
.
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. |
Fair enough. I'll undo the newer changes. Also, I was planning to add more tests, should've marked this PR as draft. |
e1e35bb
to
cb74941
Compare
No description provided.