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

Add Configuration annotation #161

Merged
merged 2 commits into from
Mar 6, 2023
Merged

Conversation

jansupol
Copy link
Contributor

@jansupol jansupol commented Mar 1, 2023

No description provided.

@jansupol
Copy link
Contributor Author

jansupol commented Mar 1, 2023

The ConfigMapping annotation used the Spec document is actually very much required when implementing the injection support to distinguish among objects being annotated by @Inject.

@ljnelson
Copy link
Contributor

ljnelson commented Mar 2, 2023

In my PR (#151) I'm trying to normalize terminology. I very much disagree with @ConfigMapping as a name/noun, since it does not match anything else in the proposed specification, but instead seems to vaguely concern configuration paths. While I believe you that at a quick glance this annotation appears necessary, I'm not sure it is the only possible solution to the set of problems it wants to answer and I think merging this in now is very premature. My two cents.

@ljnelson
Copy link
Contributor

ljnelson commented Mar 2, 2023

(In addition, for the MVP, I believe we've talked about permitting only one configuration object, so distinguishing among many would not be required.)

@jansupol
Copy link
Contributor Author

jansupol commented Mar 2, 2023

(In addition, for the MVP, I believe we've talked about permitting only one configuration object, so distinguishing among many would not be required.)

That's not about distinguishing among configuration objects, that's about other beans (such as CDI beans) being injected. The Injection Support needs to tell CDI it knows the bean and it produces the bean and it needs to skip any other bean being injected that is not a configuration bean. Hence, it needs to recognize the configuration bean somehow - by the annotation.

I tried to describe the javadoc to the annotation as abstract as possible, while the terminology is in the draft yet. The @ConfigMapping name is in the Spec document right now - we can change it for 0.2.

@ljnelson
Copy link
Contributor

ljnelson commented Mar 2, 2023

We had discussed at great length that CDI itself would do the injection. I stand by my objections.

@ljnelson
Copy link
Contributor

ljnelson commented Mar 2, 2023

From today's meeting (my understanding):

  • I still vote no on adding this annotation. But others voted yes, provided that we:
  • Rename this to Configuration and
  • Eliminate the value element

@jansupol
Copy link
Contributor Author

jansupol commented Mar 2, 2023

The advantages as named at the meeting (my understanding):

  • Readability and clarity of the code - the customer code being injected is marked as a configuration object, rather than an injection of a magic Interface without an easily spotted source of origin
  • Better performance without the creation of a bean for every interface annotated by @Inject
  • Easier implementation with the support of various injection frameworks
  • Prevent the creation of a custom bean for which a producer method has been forgotten

Disadvantages:

  • May need to rename the annotation after 0.1 to better fit the annotation purpose and future functionality

Configuration name change and removal of path.

Signed-off-by: jansupol <[email protected]>
@m0mus m0mus merged commit 3f32d61 into jakartaee:main Mar 6, 2023
@jansupol jansupol deleted the config.mapping branch March 6, 2023 21:33
@jansupol jansupol changed the title Add ConfigMapping annotation Add Configuration annotation Mar 6, 2023
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.

5 participants