jakartaee / cdi

CDI specification
Apache License 2.0
215 stars 78 forks source link

Improve specification of `BeanManager.isMatchingBean()` and `isMatchingEvent()` #748

Closed Ladicek closed 10 months ago

Ladicek commented 10 months ago

Updated the text to address both @Azquelt's comments. In the javadoc of isMatchingEvent, I changed the terms "event type" and "event qualifiers" to "specified type" and "specified qualifiers" (which are also CDI specification terms), which allowed me to be more precise in how @Default and @Any work when the event type/qualifiers are obtained from the specified type/qualifiers.

Azquelt commented 10 months ago

I think that looks better to me.

Part of my comments definitely came from not having understood how the event type and specified type were used differently (which @manovotn helpfully explained). Whether we use event type, specified type, or event object type probably doesn't matter, but since in practise you can't have a parameterized object type, defining this method in terms of the specified type probably makes it easier to understand.

Ladicek commented 10 months ago

I added one commit where I tried to make BeanContainer.isMatchingEvent() javadoc more precise. I intentionally made it an extra commit to allow comparing, but it should be squashed before merging.

manovotn commented 10 months ago

@Ladicek looks like the license check kicked in

Warning: Files with unapproved licenses: /home/runner/work/cdi/cdi/api/src/main/java/jakarta/enterprise/inject/build/compatible/spi/InvokerFactory.java

Ladicek commented 10 months ago

Oh isn't that funny. That is of course a correct observation. I think I'll submit a separate PR to fix it.

Ladicek commented 10 months ago

Rebased.

Ladicek commented 10 months ago

I reworded the 2nd commit slightly and I think this PR is now ready (after the 2 commits are squashed, of course).

Ladicek commented 10 months ago

One last tiny change after discussion with @manovotn and squashed. This should be ready to merge as is.