feature: reusable and efficient CtQueries #1090
Conversation
|
Finally we should modify Spoon factory and add there some method which creates CtQueryImpl. I suggest this: factory.createQuery() - creates unbound query I will make another PR for that |
pvojtechovsky
force-pushed
the
mergedLazyQueries
branch
from
0445295
to
639e5ce
Compare
|
I am finished with code changes. Please check the documentation. Then we can merge it. |
monperrus
changed the title
|
Really cool. I've just made a PR on your PR on the tests. If we're along the same line, I will proceed with doc.
No please don't, this PR already contains 3 baby steps, :-) (more on this later) |
monperrus
changed the title
|
I am finished with documentation fixes. It is ready for merge from my point of view |
|
I am finished too. I'm very happy that we've converged, thanks.
@surli could you review this?
I'd recommend to start with documentation (incl Javadoc), then tests,
then implementation.
|
|
Note that we'll merge after tomorrow's release.
|
- Extraction of code to CtBaseQuery to make code cleaner and to allow reuse of queries - CtFunction can return Iterator now too
Change CtConsumable#accept ... CtConsumer<R> to CtConsumer<Object>
pvojtechovsky
force-pushed
the
mergedLazyQueries
branch
from
ae4bc01
to
954c371
Compare
|
|
||
| ```java | ||
| // collecting all methods of deprecated classes | ||
| list2 = rootPackage | ||
| .filterChildren(new AnnotationFilter(Deprecated.class)) | ||
| .filterChildren(new TypeFilter(CtMethod.class)).list(); | ||
| .filterChildren(new AnnotationFilter(Deprecated.class)).list() |
There was a problem hiding this comment.
Why don't you keep an example witch chaining queries here?
There was a problem hiding this comment.
I guess @monperrus removed it.
| /** | ||
| * Recursively scans all children elements of an input element. | ||
| * The matched child element for which (filter.matches(element)==true) are sent to the next query step. | ||
| * Essentially the same as {@link CtElement#getElements(Filter)} but more powerful, because it |
There was a problem hiding this comment.
Just to be sure here: when you say filtered elements are sent to the next query step, it's not mandatory: you can just get the list of filtered elements through list() right? The meaning of your comment here is that the filterChildren can be used in the chain as it returns a CtQuery?
There was a problem hiding this comment.
the filterChildren can be used in the chain as it returns a CtQuery?
I do not understand
| * actually evaluates the query and returns all the produced elements collected in a List | ||
| * Same as {@link CtQuery#list()}, but with static typing on the return type | ||
| * and the final filtering, which matches only results, which are assignable from that return type. | ||
| * |
There was a problem hiding this comment.
So classes which are not assignable for that return type are just ignored and you cannot have a ClassCastException?
There was a problem hiding this comment.
yes, exactly
| * which is the constant input of this query. | ||
| * It is used by {@link CtElement} implementations of {@link CtQueryable}. | ||
| * | ||
| * @param <O> - the type of element which is produced by the last step of the query |
There was a problem hiding this comment.
I think you forgot to remove reference to param O.
There was a problem hiding this comment.
you are right. I removed it now
| @@ -90,126 +66,122 @@ public CtQueryImpl() { | |||
| * @param input | |||
| * @return this to support fluent API | |||
| */ | |||
| public CtQuery<O> addInput(O input) { | |||
| public CtQueryImpl addInput(Object... input) { | |||
There was a problem hiding this comment.
Why do you return the impl here and not the interface?
There was a problem hiding this comment.
Because there are some interesting methods (evaluate, ...) which are available on CtQueryImpl only and if I cast query to CtQueryImpl once then I want to have CtQueryImpl after call of addInput too. It is correct implementation of fluent API
| try { | ||
| if (stepIdx <= steps.size()) { | ||
| //process next intermediate step | ||
| AbstractStep step = steps.get(stepIdx - 1); |
There was a problem hiding this comment.
you defined a function getStep() for that.
There was a problem hiding this comment.
ok, I change the code so the getStep() is used
| * sets next {@link CtConsumer} | ||
| */ | ||
| void setNext(CtConsumer<Object> next); | ||
| private class CurrentStep implements CtConsumer<Object> { |
There was a problem hiding this comment.
Found the name CurrentStep confusing as it's not a step (and does not extends AbstractStep) but more the CurrentConsumer.
There was a problem hiding this comment.
Ok it took me a while to really understand how the whole process work and I think you should improve the doc on this class as it is central to process the different steps.
As far as I can see, this class plays as a kind of an orchestrator to move the step cursor forward, get the step, apply it and finally to call the output consumer. It could be great to add those information in the javadoc.
There was a problem hiding this comment.
javadoc added
| @@ -28,42 +28,21 @@ | |||
| import spoon.reflect.visitor.Filter; | |||
|
|
|||
| /** | |||
| * Contains the default implementation of the generic {@link CtQuery} methods | |||
| * The facade of {@link CtBaseQuery} which represents a query bound to the {@link CtElement}, | |||
There was a problem hiding this comment.
What is CtBaseQuery I assume you mean CtQuery?
There was a problem hiding this comment.
yes, CtQuery is correct. DONE
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * a step which calls Function. Implements contract of {@link CtQuery#map(CtFunction)} | ||
| * a step which calls Function. Implements contract of {@link CtBaseQuery#map(CtFunction)} |
There was a problem hiding this comment.
yes, CtQuery is correct. DONE
pvojtechovsky
force-pushed
the
mergedLazyQueries
branch
from
6e0279e
to
7705aa0
Compare
surli
changed the title
|
Ok this is ready to merge for me. @pvojtechovsky could I close #1070 too or is it a pending discussion? |
|
I have closed #1070. |
|
Ok! Then let's merge and thanks for your work 👍 |
|
Thanks @surli for the final review.
|
This is the clone of #1076 where CtBaseQuery are merged in CtQuery.
These solutions are nearly compatible from client's point of view.
Note: I have not added
CtQueryImpl#evaluate(Object, Consumer)intoCtQuery, because it is not needed on that high level of coding. The method is public and available in CtQueryImpl.I would choose #1076, because the code is little bit cleaner (shorted classes = easier maintenance), but it is up to you. Both versions are acceptable for me.
The javadoc has to be read and fixed. There are till some wrong descriptions, which does not fit to latest design. But first choose which PR is adept for merge.