[weaver] SPI decisions

[Matt Benson <gu...@gmail.com>]

Hi all, and especially Mark S.--in case I've not done it publicly enough
yet, many thanks to Mark for picking up where I had left off with the
privilizer/weaver code.  I think the two of us (others? :D) would like to
reach a point at which the library is usable, so I submit the following
talking points:


Weaver#weave():

  Currently there are separate methods for weaving a class vs. a method.  I
think it would be sufficient and cleaner to have a class weaving method
only; having provided its "interests," the Weaver can presumably figure out
what to do with a class that has been determined to match.  Next, it feels
a little odd to use a loaded Class instance to represent a class intended
for modification.  Should we just use classname here?

Weaver#getInterest():

  Rather than providing List<Class<? extends Annotation>>, I wonder whether
it would be more future-proof for this method (pluralized?) to return
Map<Class<? extends Annotation>, ElementType> to say not only which
Annotation types it is interested in, but where they are expected to be
found.  This could work comfortably with the type of search registration
APIs we've discussed for [classscan] in the future.

Weaver#configure():
  This method supplies a target directory where classes are to be found.
Perhaps too optimistically, I envision a future that could include a
[weaver] ClassLoader to apply our advice at run, rather than build, time.
Does anyone have any relevant experience that would guide us in providing
the most flexible API to permit weaving of classfiles whether on the
filesystem or otherwise?  Is it going too far to pull in [vfs] v2-core as a
dependency?

Thanks,
Matt

Re: [weaver] SPI decisions

[Matt Benson <gu...@gmail.com>]

On Sat, Jan 5, 2013 at 8:01 AM, Mark Struberg <st...@yahoo.de> wrote:

>
>
>
>
> ----- Original Message -----
> > From: Matt Benson <gu...@gmail.com>
> >
> > Weaver#weave():
> >
> >   Currently there are separate methods for weaving a class vs. a
> method.  I
> > think it would be sufficient and cleaner to have a class weaving method
> > only; having provided its "interests," the Weaver can presumably
> > figure out
> > what to do with a class that has been determined to match.
>
> Yes that might be an option. I added it because sometimes you are
> interested in just a few methods as opposed to having to scan this all
> yourself (which is redundant because xbean-finder does it much more
> efficiently).
>
>
Point taken; maybe we could combine with point 3 and still end up with a
single method which might be as simple as:

boolean weave(Class<?>(|String) type, Annotation processingAnnotation,
Map<? extends Member, ? extends Annotation> annotatedMembers);

...in the end, we're weaving the containing class even when we're *more
specifically* weaving a particular member.  The above reflects another
thought I just had--why pass the annotation type when we can pass the
actual annotation found as the 'processingAnnotation' parameter.  Again,
presumably a given Weaver knows what it's looking for.  WRT
'annotatedMembers', we might be better off to create a custom type and pass
a list of these in case we think we might want to pass the same member with
multiple annotations:

public class AnnotatedMember<A extends Annotation, M extends Member> {
  public final A annotation;
  public final M member;

...
}

boolean weave(Class<?>(|String) type, Annotation processingAnnotation,
List<AnnotatedMember<?, ?>> annotatedMembers);


>
>  > Next, it feels
> > a little odd to use a loaded Class instance to represent a class intended
> > for modification.  Should we just use classname here?
>
> Not sure, I need to look at the xbean-finder code. You need to parse the
> bytecode of the class for many things anyway and I don't think it will
> cause a file system lock.
>
>
> >
> > Weaver#getInterest():
> >
> >   Rather than providing List<Class<? extends Annotation>>, I wonder
> > whether
> > it would be more future-proof for this method (pluralized?) to return
> > Map<Class<? extends Annotation>, ElementType> to say not only which
> > Annotation types it is interested in, but where they are expected to be
> > found.  This could work comfortably with the type of search registration
> > APIs we've discussed for [classscan] in the future.
>
> Yes we could go for it. There was just no need for it yet.
>
>
> > Weaver#configure():
> >   This method supplies a target directory where classes are to be found.
> > Perhaps too optimistically, I envision a future that could include a
> > [weaver] ClassLoader to apply our advice at run, rather than build, time.
> > Does anyone have any relevant experience that would guide us in providing
> > the most flexible API to permit weaving of classfiles whether on the
> > filesystem or otherwise?  Is it going too far to pull in [vfs] v2-core
> as a
> > dependency?
>
> I fear that might be a good bit of additional work to pull off. I'd rather
> go for a version with is working for now. We can still improve it later.
>
>
I guess there's no sin in an early 2.0 ;)

Thanks!
Matt


> LieGrue,
> strub
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>

Re: [weaver] SPI decisions

[Mark Struberg <st...@yahoo.de>]

----- Original Message -----
> From: Matt Benson <gu...@gmail.com>
>
> Weaver#weave():
> 
>   Currently there are separate methods for weaving a class vs. a method.  I
> think it would be sufficient and cleaner to have a class weaving method
> only; having provided its "interests," the Weaver can presumably 
> figure out
> what to do with a class that has been determined to match. 

Yes that might be an option. I added it because sometimes you are interested in just a few methods as opposed to having to scan this all yourself (which is redundant because xbean-finder does it much more efficiently).


 > Next, it feels
> a little odd to use a loaded Class instance to represent a class intended
> for modification.  Should we just use classname here?

Not sure, I need to look at the xbean-finder code. You need to parse the bytecode of the class for many things anyway and I don't think it will cause a file system lock. 


> 
> Weaver#getInterest():
> 
>   Rather than providing List<Class<? extends Annotation>>, I wonder 
> whether
> it would be more future-proof for this method (pluralized?) to return
> Map<Class<? extends Annotation>, ElementType> to say not only which
> Annotation types it is interested in, but where they are expected to be
> found.  This could work comfortably with the type of search registration
> APIs we've discussed for [classscan] in the future.

Yes we could go for it. There was just no need for it yet.


> Weaver#configure():
>   This method supplies a target directory where classes are to be found.
> Perhaps too optimistically, I envision a future that could include a
> [weaver] ClassLoader to apply our advice at run, rather than build, time.
> Does anyone have any relevant experience that would guide us in providing
> the most flexible API to permit weaving of classfiles whether on the
> filesystem or otherwise?  Is it going too far to pull in [vfs] v2-core as a
> dependency?

I fear that might be a good bit of additional work to pull off. I'd rather go for a version with is working for now. We can still improve it later.

LieGrue,
strub


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org