review: fix, improve and document TemplateMatcher #1319
Conversation
pvojtechovsky
changed the title
pvojtechovsky
force-pushed
the
fixTemplateMatcher
branch
2 times, most recently
from
1197f35
to
c11cb5f
Compare
pvojtechovsky
changed the title
|
I am finished with commenting and few safe refactorings. I would like your feedback about correctness of comments (confirms that I understood the idea of that sometime buggy code). Does it makes sense to merge that PR then? In the mean time I will try to make some test cases - in another PRs, which will reproduce the bugs ... and then may be fix them. |
|
I love commenting PRs, that's super good for long term maintenance. However, it would be strange to merge all the "bug comments", and it would be ineffective to discuss them all here. Would you remove the bug comments from this one so that we can review and merge the valuable explanations and refactorings first? |
pvojtechovsky
force-pushed
the
fixTemplateMatcher
branch
from
96615fa
to
cb0d035
Compare
|
I kept only few simple fixes and java doc without TODOs. |
| * @param root CtClass model of {@link Template} | ||
| * @return List of all variables - only these parameters which are declared by {@link TemplateParameter}. | ||
| * It doesn't includes references to parameters annotated by {@link Parameter} | ||
| */ |
There was a problem hiding this comment.
The idea of my comment was not "just describe what algorithm does" (like your comment), but it tried to describe the purpose of this method
There was a problem hiding this comment.
I don't understand. does it return a list of variables or parameters? or a list of invocations?
There was a problem hiding this comment.
Your new comment is absolutely correct, I just suggest to keep my comment too (in improved form), because it explains purpose of this method.
It is used like this:
public TemplateMatcher(CtElement templateRoot) {
...
variables = getMethods(templateType)
...
}so it returns template variables/parameters/substitution points (please give it correct name ;-) ) ... but as written on last javadoc line, they are not all, because variables/parameters/substitution points annotated by {@link Parameter} are not included in returned list
|
Are you sure?
private List<CtInvocation<?>> getMethods(CtClass<? extends Template<?>> root) {
CtExecutableReference<?> methodRef = root.getFactory().Executable()
.createReference(root.getFactory().Type().createReference(TemplateParameter.class),
root.getFactory().Type().createTypeParameterReference("T"), "S");
List<CtInvocation<?>> meths = Query.getElements(root, new InvocationFilter(methodRef));
return meths;
}
|
|
Yes, I read this method carefully. |
|
We probably don't speak about the same thing. Let's merge it as is? |
|
Yes, I agree to merge it |
I am trying to understand how TemplateMatcher works and here are some
CtType#getReference, which is immediately followed byCtTypeReference#getDeclaringType@monperrus please have a look at it, even if it is WiP. This PR is more like list of TODO topics for future PRs. Are my changes on good way?