Skip to content
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

Allow to use all annotations as meta annotation #1012

Open
uniqueck opened this issue Apr 15, 2021 · 9 comments
Open

Allow to use all annotations as meta annotation #1012

uniqueck opened this issue Apr 15, 2021 · 9 comments

Comments

@uniqueck
Copy link
Member

Based on the intial implementation #898 for the annotation @Name it should be possible to use all remaining annotations also as meta annotation.

@uniqueck
Copy link
Member Author

I provide an implementation for that.

@uniqueck
Copy link
Member Author

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?

@robertpanzer
Copy link
Member

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.

@uniqueck
Copy link
Member Author

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.

@robertpanzer
Copy link
Member

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)

@abelsromero
Copy link
Member

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 for InlineMacroProcessor and 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.

@uniqueck
Copy link
Member Author

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.

@abelsromero
Copy link
Member

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.

@robertpanzer
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants