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.

Sign up for GitHub

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

Jump to bottom

Bump CDI version to 4.1 and place EL integration API in a new supplemental artifact #644

Merged
merged 5 commits into from
May 24, 2023
Merged

Bump CDI version to 4.1 and place EL integration API in a new supplemental artifact #644

merged 5 commits into from
May 24, 2023

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Jan 5, 2023

Fixes #613

The commits are best reviewed in isolation, and perhaps should be split into multiple PRs. Opening this one as a draft for discussion.

@Ladicek Ladicek mentioned this pull request Jan 5, 2023
Closed
@manovotn
Copy link
Contributor

manovotn commented Jan 5, 2023

Fix remnants of BeanManager.fireEvent() removal

@Ladicek your changes are not related to that. You instead modified text with BeanManager.getEvent() that method was only moved into BeanContainer. You could just alter the wording because all of the behavior stayed the same (such as rethrowing the exception).

@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 5, 2023

Fix remnants of BeanManager.fireEvent() removal

@Ladicek your changes are not related to that. You instead modified text with BeanManager.getEvent() that method was only moved into BeanContainer. You could just alter the wording because all of the behavior stayed the same (such as rethrowing the exception).

Not really. The references to BM.fireEvent() were just replaced with references to BM.getEvent(), which doesn't make sense, because getEvent() doesn't fire an event -- you have to call Event.fire() on that, and the spec text already covers Event.fire(). The references to BM.getEvent() can and should be just removed, in my opinion.

@manovotn
Copy link
Contributor

manovotn commented Jan 5, 2023

Not really. The references to BM.fireEvent() were just replaced with references to BM.getEvent(), which doesn't make sense, because getEvent() doesn't fire an event

Fair enough. I didn't recall we just replaced that.

@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 6, 2023

Not really. The references to BM.fireEvent() were just replaced with references to BM.getEvent(), which doesn't make sense, because getEvent() doesn't fire an event

Fair enough. I didn't recall we just replaced that.

See febd958 :-)

@Ladicek Ladicek force-pushed the major-version-el-split branch from 417fb00 to 9cff077 Compare January 6, 2023 09:30
@Ladicek Ladicek force-pushed the major-version-el-split branch from 9cff077 to 70fe7d3 Compare May 24, 2023 10:54
@Ladicek Ladicek changed the title Bump CDI version to 5.0 and move EL integration API to a new supplemental artifact Bump CDI version to 4.1 and place EL integration API in a new supplemental artifact May 24, 2023
@Ladicek
Copy link
Contributor Author

Ladicek commented May 24, 2023

Rebased and updated per recent discussion: this PR now bumps to 4.1, deprecates the 2 EL-related methods in BeanManager (for removal in CDI 5.0), and places the EL integration API (ELAwareBeanManager) in a new supplemental artifact.

@Ladicek Ladicek marked this pull request as ready for review May 24, 2023 10:56
@Ladicek Ladicek force-pushed the major-version-el-split branch from 70fe7d3 to 25e0bdd Compare May 24, 2023 10:57
@Ladicek
Place the EL integration API to ELAwareBeanManager in a new supplemen…
a4190e0 
…tal API artifact

The existing EL integration API in `BeanManager` is deprecated for removal.
@Ladicek Ladicek force-pushed the major-version-el-split branch from 25e0bdd to a4190e0 Compare May 24, 2023 14:08
manovotn
manovotn approved these changes May 24, 2023
View reviewed changes
Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this @Ladicek!

@manovotn
Copy link
Contributor

@starksm64 if you are fine with the changes, feel free to pull the trigger on this PR.

@Ladicek Ladicek added this to the CDI 4.1 milestone May 24, 2023
@starksm64 starksm64 merged commit 3c95b0f into jakartaee:master May 24, 2023
@Ladicek Ladicek deleted the major-version-el-split branch July 14, 2023 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@starksm64 starksm64 starksm64 left review comments

@manovotn manovotn manovotn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
CDI 4.1
Development

Successfully merging this pull request may close these issues.

Move jakarta.el-api related method to new BeanManager subclass?
3 participants
@Ladicek @manovotn @starksm64