In BasicAnnotationProcessor, correctly defer non-type elements rejected by processing steps.#1702
In BasicAnnotationProcessor, correctly defer non-type elements rejected by processing steps.#1702copybara-service[bot] wants to merge 0 commit intomainfrom
BasicAnnotationProcessor, correctly defer non-type elements rejected by processing steps.#1702Conversation
copybara-service
bot
force-pushed
the
test_main_602032802
branch
2 times, most recently
from
3610d40 to
4eef5cd
Compare
|
@AriaAdibi our internal review led to extensive but superficial changes. We'd certainly be interested in your opinion, either before or after this change is merged. |
AriaAdibi
left a comment
AriaAdibi
left a comment
There was a problem hiding this comment.
All good. Thank you for the opportunity. I just added some minor subjective opinion regarding the naming.
| * enclosing-type in its entirety should be well-formed. Since modules | ||
| * don't get annotated (and are not supported here) they can be ignored. | ||
| */ | ||
| private final Set<PackageOrTypeElementFactory> deferredElementFactories = new LinkedHashSet<>(); |
There was a problem hiding this comment.
enclosing-type: As pointed out here, if the element is a TypeElement enclosing-type might refer to a step further such element; however, in this case, we mean the element itself. Personally, I have no problem with enclosing-type being inclusive; I just wanted to point it out.
There was a problem hiding this comment.
Another subjective matter regarding the naming: The first time I read the code, I was somewhat confused about why the code has both deferredElement** and elementsDeferredBySteps. That is why I personally prefer renaming the first to signify those are the elements defered due to being ill-formed.
There was a problem hiding this comment.
Yeah, we're still debating the names. Naming Is Hard.
There was a problem hiding this comment.
Another naming caught my eye now, and I just wanted to share my opinion again. :) I still prefer my Blueprint name idea, since to me, factory usually means that something will be created. However, that is not the case here. The element is looked for (not created) that has certain characteristics.
There was a problem hiding this comment.
Naming Is Still Hard. :-) I think a Blueprint also implies creation, doesn't it? If it's something you use to find an existing thing, that's more like a Key. But also, it may actually be that a new object is created when you ask for the same method in a later round. And, in practice it's not unusual for a factory to be able to return an existing cached object rather than a new one.
| private void reportMissingElements(Set<ElementFactory> missingElementFactories) { | ||
| for (ElementFactory missingElementFactory : missingElementFactories) { | ||
| Element missingElement = missingElementFactory.getElement(elementUtils); | ||
| if (missingElement != null) { |
There was a problem hiding this comment.
Since empty is expected, I personally prefer the use of Optional instead.
There was a problem hiding this comment.
I would be inclined to agree if we could use Optional.ifPresentOrElse here. Then we wouldn't have the ugly .ifPresent...get pattern. Unfortunately we're still building on Java 8 and ifPresentOrElse was only added in Java 9.
copybara-service
bot
force-pushed
the
test_main_602032802
branch
from
4eef5cd to
08218d6
Compare
copybara-service
bot
changed the title
BasicAnnotationProcessor, correctly defer non-type elements referencing missing types.BasicAnnotationProcessor, correctly defer non-type elements rejected by processing steps.
copybara-service
bot
force-pushed
the
test_main_602032802
branch
from
08218d6 to
9e5c0ec
Compare
copybara-service
bot
force-pushed
the
test_main_602032802
branch
2 times, most recently
from
b64af3f to
fd767a5
Compare
copybara-service
bot
force-pushed
the
test_main_602032802
branch
from
fd767a5 to
e20db25
Compare
2 participants
In
BasicAnnotationProcessor, correctly defer non-type elements rejected by processing steps.See #1348 for details. If a processor is triggered by a method annotation, for example, and the processor rejects the method until the next round, then we should defer processing of just that method, not the entire class containing it.
Closes #1348.
RELNOTES=
BasicAnnotationProcessorhas improved handling of deferring processing of methods and other non-type elements. Thanks to @AriaAdibi for this contribution!