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

ISPN-1261 Implemented missing CDI events #2845

Closed
wants to merge 1 commit into from

Conversation

slaskawi
Copy link
Contributor

@slaskawi slaskawi commented Sep 5, 2014

Hi!

Please take a look at the implementation of missing events: https://issues.jboss.org/browse/ISPN-1261.

It turned out, that there was a problem with Weld implementation and generic type of the events (EventImpl<K,V>). IMO Weld implements CDI spec correctly here (CDI spec 10.3), so I introduced a small wrapper around Events and, to make it type safe - it's an inner class of adapters.

I'm not sure if we should supporting deprecated CacheEntriesEvictedAdapter. I removed it, but perhaps we should leave it?

Best regards
Sebastian


/**
* CDI does not allow parametrized type for events (like <code><K,V></code>). This is why this wrapped needs to be
* introduced. To ensure type safety, this needs to be linked to parent class (in other words this class can not
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I understand why this class cannot be an inner static class, it doesn't seem to use any externally defined instances. It simply takes a decoratedEvent and delegate to it. Also, I don't see any references to CDICacheEntriesEvictedEvent anywhere else, so maybe it could be even be private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is a very good explanation why we can't make it static: http://stackoverflow.com/questions/12334159/jls7-generic-inner-class-documentation-error/12334315#12334315

Actually we need K and V in here CacheEntriesEvictedEvent<K, V>. These generic type information must be passed here from enclosing class. Another solution would be simply to discard all generics in this inner class and then we might have made it static.

And for making it private: +1 :)

Copy link
Member

Choose a reason for hiding this comment

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

Good point, that's Ok then.

@galderz
Copy link
Member

galderz commented Sep 12, 2014

Excellent work @slaskawi :), just some small comments on the code. As long as CacheEntryEvicted is present in core/ module and since CacheEntryEvictedAdapter was already present in the CDI module, we need to continue supporting it. Once CacheEntryEvicted is removed from core, we can remove related work in CDI.

@slaskawi
Copy link
Contributor Author

Ok now it's clear - CacheEntryEvicted needs to be preserved.... at least for now...

@galderz
Copy link
Member

galderz commented Sep 18, 2014

Integrated, thanks @slaskawi :)

@galderz galderz closed this Sep 18, 2014
@slaskawi slaskawi deleted the ISPN-1261 branch January 22, 2015 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants