asciidoctorj
asciidoctorj copied to clipboard
Allow to use all annotations as meta annotation
Based on the intial implementation #898 for the annotation @Name it should be possible to use all remaining annotations also as meta annotation.
I provide an implementation for that.
Should it possible to support multiple annotations at once? This means, if i scan for an existence of @Location annotations and get more than one, should i register the same extensions multiple times. For the @Name annotation we have decided so.
Or let me ask it another way, is there an annotation where we should do it differently?
I think except for The @Name annotation extensions should not be registered multiple times.
Imo as a general rule an annotation defined on a subannotation should always shadow the same annotation type from its superinterface. If there are 2 concurring values from 2 parallel super-annotations I would be fine with ignoring them entirely and expecting the annotations to be defined on a closer level.
I think this makes the code really complicated. Isn't it better to leave that up to the user to decide how to use it? So we can use the same implementation for @Name and all other annotations, or exist an annotation where it is complicated.
Intuitively I think that these annotations should work similar to other mechanisms that users might already know. If I understand for example correctly how Spring works I think an annotation on the class itself or on an annotation overwrites the same annotation type coming from a super annotation. (Summoning @abelsromero as he might know best now :D)
If I understand for example correctly how Spring works I think an annotation on the class itself or on an annotation overwrites the same annotation type coming from a super annotation.
It's a case by case to me. Basically the annotation is metadata and you can use them whatever way you want. Same way we made @Name a meta-annotation by using annotationType() from other annotations, we could just getSuperclass() to also search for it in parent classes or interfaces.
Said that, obviously following Java conventions most frameworks do not treat annotations in sub and parent class as repeatable, and instead they take the closest.
--
To the topic at hand, I think a general all-repeatable solution would not the help the users. Questions about extension writing are the most common thing in the support channels. Also, I am not sure we need to create a proxy class for each annotation, for example for DefaultAttribute we configure them all in a single one. If only for performanca I think it's worth paying attention to each case. Truth, I'd like to see a case by case approach in which each one has clearly defined:
- To what extension type is applicable. That would allow to provide log messages telling users if they missconfigured an extension.
- Whether it's repeatble. To avoid missconfigurations which forces us to decide whether we ignore values or abort with error.
- Implementation model: single proxy class or multiple. But maybe that's too much now, so maybe there's a middle ground, for example keeping current inplementation and only treating points second and third.
I reviewed the current annotations and we have:
-
@Contexts: probably makes sense to be repeatable and maybe apply all to a single instance. But I am not sure about the later, I need to dig more into the Ruby internals. -
@DefaultAttribute: this is already repeatble using@DefaultAttributes(plural) and sets all values to the same instance. Here is obvious we should make the annotation repeatable, so users can just use the singular one. -
@Format: only applies forInlineMacroProcessorand could be repeatable but probably registering two instances of the same extension. -
@Location: applies to DocinfoProcessor, could be repeated, but would require registering the same annotation for each annotation. -
@PositionalAttributes: already an array if String, and doesn't make sense to be repeatable.
So the idea is to implement it the same way as spring it have. So I would like use the same behaviour for the @Name annotation. Is that ok? So I would came back with a suggestion.
I still think it's a case-by-case and would require special treatments, a general approach won't be possible. @Location makes sense, but for instance:
- A BlockProcessor that via meta ends up with 2 Names and 2 Contexts, requires the creation of 2 proxies (1 for each name) and the merging of the respective values from Contexts (context is String[]).
- Same scenario for a TreeProcessor doesn't make sense for instance, because those annotations won't apply.
PS: I realized there's another annotation I missed, @ContentModel, it's located in package org.asciidoctor.ast. Tbh, that doesn't seem right.
I agree with @abelsromero: Every annotation needs to be treated individually.
About making @DefaultAttribute repeatable I'd like to push that out to AsciidoctorJ 2.6.0 so that we can release AsciidoctorJ 2.5.0 including Asciidoctor 2.0.14 now.