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 @Inject instead of @Context with CDI #4749

Merged
merged 3 commits into from
Apr 15, 2021

Conversation

jansupol
Copy link
Contributor

Signed-off-by: jansupol jan.supol@oracle.com

Signed-off-by: jansupol <jan.supol@oracle.com>
</parent>
<modelVersion>4.0.0</modelVersion>

<artifactId>jersey-cdi-inject</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be nice to get the word context in the artifact ID. CDI is already part of the group ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the word cdi corresponds to other modules, all of them have cdi1x in the artifact id. The 1x is no longer used though, we use 2x (and even 3x in Jersey 3), so I left the 1x behind. I thought about jersey-cdi-context-inject, but I am not sure it is any better. Do you have any particular name in mind?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon further consideration, jersey-cdi-jaxrs-inject seems to be a more descriptive name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jansupol as "jaxrs" is the old Java EE name, what about considering the new Jakarta name? E.g. "Jakarta Rest" or just "rest" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arjantijms Thank you to chime in. The naming conventions seem to be the hardest on the overall work. I agree that "jaxrs" is not the best we could use in Jakarta EE. For Jersey 2.x it is far more descriptive than any other name I heard. For 3.x, changing to "rest" sounds a bit ambiguous. One can create REST endpoints with JAX-WS, for instance. "jakarta-rest" on the other hand sounds too long. Ideally, the name should be the same for 2.x and 3.x. jersey-cdi-rs-inject perhaps? I am open to any suggestions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jersey-cdi-rs-inject it is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JAX-RS is neither old nor new nor forbidden or obsolete. Most people understand what JAX-RS means and clearly the technology itself is mostly referred to still as JAX-RS. As I explained several times, it is officially still allowed and legal to keep using the word JAX-RS anywhere. One the specification title (hence, reference to the specification, not the technology) must be named "Jakarta RESTful Web Services". So it would have been absolutely ok that you keep "jaxrs" in the name if you like to do that. Just to make things clear, as there is much FUD about this. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely for a discussion with a larger audience. The official Jakarta documents do not like JAX prefix. I know you keep saying you have the approval, but no one has seen it. No matter the legal stuff, the javax based vs jakarta based Jersey differences are better explained to the customers when each has a different name, JAX-RS for Java EE 8, Jakarta REST for Jakarta EE 9.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't get me wrong - I am totally OK with your decision. I just wanted to make clear that there is absoutely no legal reason that forces you to get rid of the word "JAX-RS". The document you correctly refer to just says "SHOULD" not "MUST", and it clearly asks for the change only for documents etc, but not for source code and other technical stuff:

For names of Jakarta EE projects, specifications, test suites, and other related materials, the full project name/title and any scope statement SHOULD not incorporate an identified acronym.

The document even allows the use of the "old" acronyms explicitly for technical stuff:

A URL, repository name, package name, class name, or method name MAY include the acronym to the right of a string identifying Eclipse or Jakarta as the origin.

It is completely up to the Jersey committers to decide whether they still use the acronym JAX-RS or not. No instance at the Eclipse Foundation or at Oracle explicitly forbids it. The document you refer to makes this 100% clear, so I am not sure why you say, "no one has seen it": It is written THERE. THAT document IS the final answer. So there is no more FUD. :-)

Copy link
Contributor

@spericas spericas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your other proposal seems better to me, I like the idea of having context in the name, the original one is way too generic IMO. It's a really minor point, though.

Signed-off-by: jansupol <jan.supol@oracle.com>
Signed-off-by: jansupol <jan.supol@oracle.com>
Copy link
Contributor

@tomas-langer tomas-langer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jansupol jansupol merged commit 06b1a6c into eclipse-ee4j:master Apr 15, 2021
@jansupol jansupol deleted the cdi-inject branch April 15, 2021 17:27
@jansupol jansupol self-assigned this Apr 15, 2021
@jansupol jansupol added this to the 2.34 milestone Apr 15, 2021
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 this pull request may close these issues.

6 participants