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-420 define trim also in beans xsd. #348

Merged
merged 1 commit into from
Dec 5, 2016
Merged

Conversation

tremes
Copy link
Contributor

@tremes tremes commented Nov 30, 2016

Attempt to define trim in beans_2_0.xsd. This should be just an empty element if I am not wrong.

@@ -353,5 +354,14 @@
</xs:choice>
</xs:complexType>
</xs:element>
<xs:element name="trim" type="xs:string" fixed="">
Copy link
Contributor

Choose a reason for hiding this comment

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

I was just thinking if we could/should add some restrictive attributes here. Such as limiting min/max occurrences?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to force occurrence to 1. Otherwise looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I was thinking about this as well. So then it makes sense to change it as above. I don't think it makes sense to allow multiple occurences of e.g <scan>.

I don't know xsd much, but now each of these elements could be specified only once or do somebody have better idea how to define this???

Copy link
Contributor

Choose a reason for hiding this comment

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

My science in xsd is not very high, but my friend Google told me that:

The all element specifies that the child elements can appear in any order and that each child element can occur zero or one time.

(See ref here)

So minOccurs attributes are useless here IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you are talking about lines 81-85 then your suggestion doesn't seem to be correct in this case because the minOccurs default value is 1 here AFAIK.
I was experimenting in Idea and if you omit minOccurs then you really will need to define all the elements.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right @tremes. Pr is good for me then

Copy link
Contributor

@antoinesd antoinesd left a comment

Choose a reason for hiding this comment

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

LGTM

@tremes
Copy link
Contributor Author

tremes commented Dec 5, 2016

My concern is that with this approach we could potentially break beans.xml files e.g with multiple <alternatives> elements, because I guess this was not validated before but need to check.

@antoinesd
Copy link
Contributor

@tremes it looks like a corner case and can't happen without user modification.
It's not a backward compatibility issue IMO: if the user has this (wrong but valid in 1.1) pattern and doesn't touch the beans.xml file, it will continue to be validated against 1.1 xsd and will continue to work (if Weld 2.X accept it today)

@tremes
Copy link
Contributor Author

tremes commented Dec 5, 2016

@antoinesd ok you are probably right. Thanks

@manovotn
Copy link
Contributor

manovotn commented Dec 5, 2016

+1 from me

@antoinesd antoinesd merged commit 5703358 into jakartaee:master Dec 5, 2016
@Emily-Jiang
Copy link
Contributor

I cannot see my comments were taken into consideration:

Bean defining annotations include the any scope annotations. Therefore, it makes more sense to delete "or any scope annoation".
Any comments?

@mkouba
Copy link
Contributor

mkouba commented Dec 6, 2016

@Emily-Jiang Bean defining annotations only contain normal scopes and @Dependent. We'd like to support @Singleton and other pseudo-scopes as well.

@Emily-Jiang
Copy link
Contributor

@mkouba It is confusing to add the support for @singleton and other pseudo-scopes as they are not beans. What user case do you have in mind?

@mkouba
Copy link
Contributor

mkouba commented Dec 6, 2016

Actually, @Singleton is supported by both OWB and Weld (although it's not clearly defined in the spec).

@Emily-Jiang
Copy link
Contributor

Then we should push to get @singleton added to CDI-2.0 spec as bean defining annotations. -1 on adding other pseudo-scopes though.

@tremes
Copy link
Contributor Author

tremes commented Dec 6, 2016

Problem is that @singleton annotation is not from CDI API but from javax inject API. Pseudo scopes are necessary IMO.
Btw I don't understand why we are discussing here when this PR doesn't change or update this wording and is closed.

@antoinesd
Copy link
Contributor

@Emily-Jiang take a look at https://issues.jboss.org/browse/CDI-377 resolved in CDI 1.2

@Emily-Jiang
Copy link
Contributor

@antoinesd thanks. I am not comfortable that we support these annotations as beans without being stated in the spec. This is another discussion.
@tremes We still need to discuss the remaining issues even if it is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants