Skip to content

Conversation

nuboat
Copy link

@nuboat nuboat commented Dec 9, 2014

I use AOP on my project but I don't see the way to bind interceptor.
I just add one function and one test for Scala style interceptor binding.

From

bindInterceptor(Matchers.any(), Matchers.annotatedWith(classOf[Logging]), new LoggingInterceptor())

To

bindInterceptor[Logging, LoggingInterceptor]

Add bindInterceptor[T <: Annotation : Manifest, T2: Manifest] in
ScalaModule
Copy link
Member

Choose a reason for hiding this comment

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

I would like to see Matchers as a parameter to bindInterceptor. Matching all objects is not always the right answer.

Copy link
Member

Choose a reason for hiding this comment

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

I would also add a call to requestInjection(interceptor). It's harmless for objects that have no injections.

Copy link
Author

Choose a reason for hiding this comment

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

What's "requestInjection(interceptor)" this mean?

Copy link
Author

Choose a reason for hiding this comment

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

Plesae help me one thing,
I don't know how to pass Matcher as a parameter.

I tried bindInterceptor[m: Matcher[_ >: Class[_]], A <: Annotation : Manifest, I <: MethodInterceptor : Manifest]

It show compile error "com.google.inject.matcher.Matcher[_ >: Class[_]] does not take type parameters"

Copy link
Member

Choose a reason for hiding this comment

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

requestInjection is a method call that you can make in the binder, maybe you need myBinder.requestInjection.

If your interceptor has any @Inject setters then it will invoke them when the injector is created.

About the matcher as a parameter:
I'm trying to think about how to structure this for other people to use and I'm not sure the best way to generalize it. Github sent my comment off right away without me having a chance to think twice about it. It's a little odd because technically speaking all three of the parameters are just that, parameters. There's very little that you can do generalize them.

What I had meant was to do something like this:

def bindInterceptor[A ..., I ...] { 
  bindInterceptor[A, I](Matchers.any())
}

def bindInterceptor[A ..., I ...](m: Matcher[_ >: Class[_]]) {
  ...
}

Copy link
Author

Choose a reason for hiding this comment

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

Wow really thank you.

@nbauernfeind
Copy link
Member

On second thought; this seems relatively limiting. I'm not really convinced about this change. I don't think this shortcut would apply to many people.

What are your thoughts @tsuckow?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, you've got to stick to the copyright already in the project. Please omit this block.

Copy link
Author

Choose a reason for hiding this comment

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

Ok

@nbauernfeind
Copy link
Member

I've been reading through https://github.com/google/guice/wiki/AOP and the uses inside of guice (including tests).

There are some other common patterns for AOP:
All methods in a class that has a specific annotation.
All methods across all classes that have a specific annotation.
All annotated methods in your custom package prefix.

Also the original method takes a list of interceptors; presumably preserving order when invoking them.

These things just make me shrug a little about whether or not this is something we'd want to include.

@nbauernfeind
Copy link
Member

So I'm trying to think about a generalization of this that is scala friendly but doesn't lose functionality.

def bindInterceptor[I <: Interceptor : Manifest](classMatcher: Matcher[_ >: Class[_]] = Matchers.any(), methodMatcher: Matcher[_ >: Class[_]] = Matchers.any()) {
  ...
} 

Maybe with a use case of:

bindInterceptor[AOPI](methodMatcher = annotatedWith[AOP])

What do you think of that?

@nuboat
Copy link
Author

nuboat commented Dec 10, 2014

That's cool, I'm submit new pull request with change.

Peerapat Asoktummarungsri added 2 commits December 10, 2014 18:29
@nbauernfeind
Copy link
Member

No, that's not necessary. Since you published your changes to your fork's bindInterceptor branch it already updated this pull request. I'll take a look at this today! Thank you tons for being responsive to feedback!

Copy link
Member

Choose a reason for hiding this comment

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

Typo: interceptor

@nbauernfeind
Copy link
Member

Thanks! This change is awesome! I think this usage pattern is definitely a net win. I particularly like the readability improvement over the java version.

@tsuckow Up to you for additional comments and the merge. I'm 👍 !

@tsuckow
Copy link
Member

tsuckow commented Jan 7, 2015

Published a new snapshot release including this change.

@tsuckow tsuckow added this to the 4.0.0 milestone Jan 7, 2015
@tsuckow
Copy link
Member

tsuckow commented Jan 7, 2015

This pull request is against master, it should have been against develop. I have merged it into develop and am closing this request.

@tsuckow tsuckow closed this Jan 7, 2015
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.

3 participants