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

Meta-annotations support #14

Closed
thmasker opened this issue Mar 5, 2024 · 31 comments · Fixed by #15
Closed

Meta-annotations support #14

thmasker opened this issue Mar 5, 2024 · 31 comments · Fixed by #15

Comments

@thmasker
Copy link
Contributor

thmasker commented Mar 5, 2024

Hi, I'm using this library's @Builder in quite a lot of classes with the same arguments:

@Builder(setterPrefix = "with", style = BuilderStyle.STAGED, factoryMethod = "builder")

I was thinking of creating an annotation to avoid this code repetition, but I see this annotation cannot be used as meta-annotation. I lack enough knowledge to know if it is possible to implement this functionality, but I think it would be really nice if possible

@skinny85
Copy link
Owner

skinny85 commented Mar 6, 2024

Hi @thmasker,

thanks a lot for opening the issue!

To summarize, you'd want to do something like:

@Target(ElementType.TYPE)
@Retention(RetentionPolicy.SOURCE)
@Builder(
    setterPrefix = "with",
    style = BuilderStyle.STAGED,
    factoryMethod = "builder")
public @interface MyBuilder {
}

And then use it like so:

@MyBuilder
public MyValueClass {
    // ...
}

Did I understand your intentions correctly?

Thanks,
Adam

@thmasker
Copy link
Contributor Author

thmasker commented Mar 6, 2024

That’s exactly what I wanna do

@skinny85
Copy link
Owner

skinny85 commented Mar 6, 2024

Sweet. I think that's a very reasonable ask, and a lot of other annotation processor libraries work this way.

I'll work on this feature this month. I'll update this issue as I know more.

@thmasker
Copy link
Contributor Author

thmasker commented Mar 6, 2024

Thanks! I’m open to contribute as well…

@skinny85
Copy link
Owner

skinny85 commented Mar 6, 2024

Thanks for being open to that 🙂. Do you have any experience with implementing meta-annotations? I have to confess I don't, but I can do some research 🙂.

@thmasker
Copy link
Contributor Author

thmasker commented Mar 6, 2024

I don't either, but I will do some research too, and try to implement it

@thmasker
Copy link
Contributor Author

thmasker commented Mar 6, 2024

I've done a PR (#15). I tried to link the issue, but was not able to

@skinny85
Copy link
Owner

skinny85 commented Mar 7, 2024

Thank you! I've commented on the PR, and edited its title and description to link it with this issue.

skinny85 pushed a commit that referenced this issue Mar 8, 2024
Fixes #14

---------

Co-authored-by: Diego Pedregal <diego.pedregal@geo-satis.com>
@skinny85
Copy link
Owner

This has been fixed in Jilt release 1.5.

I'm resolving this issue, please comment if you run into this problem again, and I'll be happy to reopen.

@thmasker
Copy link
Contributor Author

Thanks! Just so you know, I'm right now testing it and it works. However, there is a case in which it isn't. Since I'm using the same builder annotation for several projects, I've created my own builder annotation on a common Maven project, shared between all the rest of the projects. In this case, builders are not generated.

@skinny85
Copy link
Owner

@thmasker would you mind creating a small GitHub repo with a reproduction? I’d be easier for me to investigate that way.

@skinny85 skinny85 reopened this Mar 25, 2024
@thmasker
Copy link
Contributor Author

I will, asap

@skinny85
Copy link
Owner

Thank you!

@thmasker
Copy link
Contributor Author

thmasker commented Mar 25, 2024

jilt-common includes the common builder
jilt-test tries to use it

@thmasker
Copy link
Contributor Author

Another interesting feature would be to add @Documented in both @Builder and @BuilderInterfaces. At least I'd find it useful in the case of meta-annotations. Anyway, this is a minor update and I'm already happy with the library. Just so you may consider it...

@skinny85
Copy link
Owner

skinny85 commented Mar 25, 2024

Remind me, what does @Documented do again? 😛

@thmasker
Copy link
Contributor Author

thmasker commented Mar 25, 2024

It would make @Builder and/or @BuilderInterfaces annotations to appear in JavaDocs in its annotated elements. I think it would be really useful if Jilt's annotations are used as meta-annotations. This is a more practical explanation.

A more technical explanation can be found here: https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/annotation/Documented.html

@thmasker
Copy link
Contributor Author

A thought just popped in right before going to bed: it might have to do with the retention policy. I will test tomorrow if possible

@skinny85
Copy link
Owner

It would make @Builder and/or @BuilderInterfaces annotations to appear in JavaDocs in its annotated elements. I think it would be really useful if Jilt's annotations are used as meta-annotations. This is a more practical explanation.

A more technical explanation can be found here: https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/annotation/Documented.html

Sure. We can add it, I don't have any objections to that.

@thmasker
Copy link
Contributor Author

thmasker commented Mar 26, 2024

Follow up: I tested it and obviously with RetentionPolicy.SOURCE annotations are lost after compiling the common project, so no processing takes place on dependent projects. I also tested it with RetentionPolicy.CLASS and RetentionPolicy.RUNTIME. In these cases, annotations are preserved, but no processing is made. I guess that's because the common project is compiled and dependent projects use those compiled files. So far, I don't see how to fix this, although I will continue to investigate...

@skinny85
Copy link
Owner

Actually, CLASS preserves the annotations in the compiled .class files (hence the name).

@thmasker
Copy link
Contributor Author

You are right! I edited the comment to make sense. It was a typo

@skinny85
Copy link
Owner

I did some digging on my own, and the problem is that the process() method of the annotation processor is not even called when a class is annotated just with @MyBuilder (without @Builder) in the consuming project.

I'm wondering whether the solution is that you have to annotate your class with both, but the Jilt annotation processor recognizes that a class is "double annotated" (through the meta-annotation) with @Builder, and it merges the attributes correctly (so, @MyBuilder attributes will take precedence over @Builder one).

So, given:

@Builder(setterPrefix = "with", style = BuilderStyle.STAGED, factoryMethod = "builder")
public @interface MyBuilder {
}

If you then have:

@MyBuilder
@Builder
public final class MyClass {
    // ...
}

Then it's like if you annotated MyClass with @Builder(setterPrefix = "with", style = BuilderStyle.STAGED, factoryMethod = "builder"). So, while we do need to have 2 annotations, the attributes of @Builder are not repeated, so @MyBuilder does serve its basic purpose.

Thoughts on this @thmasker?

@thmasker
Copy link
Contributor Author

To be honest I'd rather redo my custom annotation per project. I see your proposal more like a tweaking. It seems there is no other way at the moment, tho. But I'd really like to know your opinion on it as well since you are far more experienced than me. Would you apply that solution on your projects?

@skinny85
Copy link
Owner

I agree with you 🙂. While it's not the worst, it is a little awkward. I would probably also just define the annotation inside my project, and abandon the idea of putting it into a shared library.

@thmasker
Copy link
Contributor Author

Maybe coding a processor for the custom annotation in the shared library would work. Although it is also a bit of work, maybe simply extending or copying Jilt's would be enough... now this is a challenge for me haha. I can't rest until I find a solution.

@skinny85
Copy link
Owner

skinny85 commented Mar 29, 2024

I think that's absolutely insane, but hey, go for it - don't let me stop you! 😃

@skinny85
Copy link
Owner

@thmasker I assume with #20 done, we can safely close this issue.

Let me know if you run into any problems related to this, and I'd be happy to reopen.

@thmasker
Copy link
Contributor Author

Hi @skinny85. First of all, thanks for releasing the new version! But I'm afraid this problem is not solved... Meta-annotations still fails to work when using a common library for multiple projects

@skinny85
Copy link
Owner

Well, I think the only thing we can do here is #14 (comment), and we both agreed we probably don't want to go there, so I'm not sure there's much left to do here...

@thmasker
Copy link
Contributor Author

I guess you are right

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

Successfully merging a pull request may close this issue.

2 participants