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

CDI-627 also re-enable the rules of CDI-1.0 #309

Merged
merged 3 commits into from Oct 19, 2016

Conversation

struberg
Copy link
Contributor

This is for fixing a backward-incompatible change we introduced in CDI-1.1.

The whole reason for verifying the entries is to ensure that a user
doesn't have typos or list classes which do not exists or are no
'candidates' for being an Alternative at least.
So it's all about preventing user errors. Disallowing to veto() @Alternative
classes (which was perfectly allowed in CDI-1.0) was overreaching.

This is for fixing a backward-incompatible change we introduced in CDI-1.1.

The whole reason for verifying the <class> entries is to ensure that a user
doesn't have typos or list classes which do not exists or are no
'candidates' for being an Alternative at least.
So it's all about preventing user errors. Disallowing to veto() @Alternative
classes (which was perfectly allowed in CDI-1.0) was overreaching.
@manovotn
Copy link
Contributor

LGTM

@@ -74,7 +74,12 @@ An alternative is selected for the bean archive if either:
----

Each child `<class>` element must specify the name of a bean class of an alternative bean.
Copy link
Contributor

Choose a reason for hiding this comment

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

If we go for it (I think it's basically ok but on the other hand we are going to confuse the users again little bit) then this sentence needs some change as well IMHO. Because when I veto it then it's not a bean anymore. So just something like?:
Each childelement must specify the name of a class of an alternative bean candidate. or
Each childelement must specify the name of a class which is annotated with @Alternative.
WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the first sentence and merged it with the 2nd. Should make things clearer. The first sentence was redundant with the paragraph above anyway.

The first sentence is more strict and actually redundant.
If there is no bean whose bean class has the specified name, or if no bean whose bean class has the specified name is an alternative, the container automatically detects the problem and treats it as a deployment problem.
For each child `<class>` element the container verifies that either:

* a class T exists which is annotated with `@Alternative` or contains a producer field or producer method which is annotated with `@Alternative`, or
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make T T instead since its code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, txs!

@antoinesd
Copy link
Contributor

LGTM

@antoinesd antoinesd merged commit 7a8fe53 into jakartaee:master Oct 19, 2016
@tremes
Copy link
Contributor

tremes commented Oct 21, 2016

I am looking again at this because of TCK assertion update and I am affraid that this wording excludes alternative stereotypes.

@struberg
Copy link
Contributor Author

good catch @tremes there should be another bullet, similar to the first one - just for alternative stereotypes

@struberg struberg deleted the CDI-627 branch October 21, 2016 14:28
@Emily-Jiang
Copy link
Contributor

Can this be fixed in Weld?

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.

None yet

6 participants