-
Notifications
You must be signed in to change notification settings - Fork 77
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
Separate the language model to a dedicated module #553
Conversation
83b005d
to
5986731
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped some comments.
I have no idea about those licenses and what needs to be there. I guess you just copied what other modules use ATM?
<version>4.0.0-SNAPSHOT</version> | ||
</parent> | ||
|
||
<artifactId>jakarta.enterprise.lang-model</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to prepend jakarta.enterprise
?
Just having lang-model
sounds a lot better to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at Maven Central, you'll find both jakarta.enterprise:cdi-api
and jakarta.enterprise:jakarta.enterprise.cdi-api
. I don't know if that was a mistake or not, but it seems that the shorter form was considered and rejected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that but TBF I don't know what the history was. I just thought it would be more readable the shorter it is.
<artifactId>jakarta.enterprise.lang-model</artifactId> | ||
<packaging>jar</packaging> | ||
|
||
<name>CDI Language Model</name> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we want to make this potentially usable elsewhere, I'd say it is just Language Model Abstraction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to avoid something like "Jakarta Language Model", because that's not what it is at the moment, and just "Language Model" seems strange. I attribute this to CDI on a few places, but it's worded in a way that generalizing to Jakarta should be straightforward (if that's what it ends up being).
<packaging>jar</packaging> | ||
|
||
<name>CDI Language Model</name> | ||
<description>Build Compatible (Reflection-Free) Java Language Model for CDI</description> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, would drop CDI. Alternatively, change the wording to say that CDI is an example of framework leveraging this language model?
Also add `package-info.java` and `overview.html` files, so that the generated javadoc looks a bit better.
5986731
to
df6bd5f
Compare
@Ladicek feel free to merge this. My comments were minor things anyway. |
Thanks! |
No description provided.