-
Notifications
You must be signed in to change notification settings - Fork 41
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 new method IQueryResult.stream() #312
Add new method IQueryResult.stream() #312
Conversation
@@ -8,4 +8,12 @@ | |||
</message_arguments> | |||
</filter> | |||
</resource> | |||
<resource path="src/org/eclipse/equinox/p2/query/IQueryResult.java" type="org.eclipse.equinox.p2.query.IQueryResult"> |
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.
IQueryResult is not marked with @noimplement/@noextend
but I wonder if clients are really supposed to create own implementations?
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.
Tycho has some implementations of IQueryResult
, I don't see why one should not allow this?
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.
People do what they're not supposed to do, so if they're allowed, one can assume that someone will to it. 😱
What about a default method that does toSet().stream() so that by default there is always an implementation?
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.
OK with custom queryable this makes sense.
Tycho even already has a stream() method in its own implementation, but it only needs to be made public when depending on the next Eclipse Release, so I think this is fine.
https://github.com/eclipse-tycho/tycho/blob/master/p2-maven-plugin/src/main/java/org/eclipse/tycho/p2maven/ListQueryable.java#L80-L83
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.
IQueryable
already implements Iterable
so using the iterator for default implementations seems a better choice to prevent creation of an intermediate set.
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.
What about a default method that does toSet().stream() so that by default there is always an implementation?
That's possible. But as Christoph suggested it could use the Spliterator
of the IQueryResult
, like it is currently done in QueryResult
.
Iterable
itself unfortunately does not provide a stream() method (if it would this PR would be pointless).
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.
What about a default method that does toSet().stream() so that by default there is always an implementation?
That's possible. But as Christoph suggested it could use the
Spliterator
of theIQueryResult
, like it is currently done inQueryResult
.
Done.
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 fine.
717690c
to
9a36ef7
Compare
9a36ef7
to
a55a707
Compare
This improves the interoperability of IQueryResult and Java's Stream-API.
a55a707
to
f507ce9
Compare
This improves the interoperability of
IQueryResult
and the Java's modern Stream-API.Additionally Clean-up
IQueryResult
and its implementations in a separate first commit.