You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@onami.apache.org by Simone Tripodi <si...@apache.org> on 2013/03/11 21:18:26 UTC
[lifecycle] polishing org.apache.onami.lifecycle.core.ListBuilder
Hi Jordan/all,
while reviewing the incredible work that Jordan just checked-in in
[lifecycle] module, I got interested by ListBuilder class, I have few
questions/observations:
* calling it ListBuilder is a little confusing, because the name
reminds me a collections general purpose class, I'd rename it to
LifecycleAnnotationsListBuilder;
* the adapted List of annotation should be IMHO a LinkedHashSet,
otherwise users could specify the same type twice in the list and
drive the lifecycle engine to invoke twice the same method in
different phases - IIUC this is not the desired behaviour, or is it?
* according to previous point,
org.apache.onami.lifecycle.core.LifeCycleModule should be modified in
order to accept a more appropriate data structure.
WDYT?
TIA, all the best,
-Simo
http://people.apache.org/~simonetripodi/
http://simonetripodi.livejournal.com/
http://twitter.com/simonetripodi
http://www.99soft.org/
Re: [lifecycle] polishing org.apache.onami.lifecycle.core.ListBuilder
Posted by Jordan Zimmerman <jo...@jordanzimmerman.com>.
I have OCD about that stuff. I hate seeing any yellow warnings in my IDE. Also, having to add a comment to fix the warning seems wrong.
On Mar 14, 2013, at 12:33 PM, Simone Tripodi <si...@apache.org> wrote:
> personally I am not so worried about that warnings, they just make
> some noise - feel free to include them or not.
Re: [lifecycle] polishing org.apache.onami.lifecycle.core.ListBuilder
Posted by Jordan Zimmerman <jo...@jordanzimmerman.com>.
The biggest annoyance is the way @Override is handled. They made the mistake of disallowing it on Interfaces in Java 5. This is fixed in Java 6. But, of course, this is just a nit.
On Mar 14, 2013, at 12:33 PM, Simone Tripodi <si...@apache.org> wrote:
>> Yeah - the big problem is that this generates a warning:
>>
>> Arrays.asList( WarmUp.class, PostConstruct.class )
>>
>
> personally I am not so worried about that warnings, they just make
> some noise - feel free to include them or not.
>
> I'd suggest anyway to support Iterable<E> rather than just List<E> for
> accepting annotation types, like Guice.createInjector()[1] does
>
>> I'm glad that this is fixed in Java 7. BTW - why are we targeting Java 5? Can we at least move to Java 6?
>
> do you need some JVM feature that require JDK6? if yes - please
> specify the reasons - you can override the ${javac.src.version} and
> ${javac.target.version} properties in the component pom.
> if not, ... which is the sense to upgrade to 6? :) having APIs still
> be backward compatible with JDK5 is still fine since Guice targets it
> - and immagine I still have
> customers which run their application on JDK1.4! :D
>
> best,
> -Simo
>
> [1] http://google-guice.googlecode.com/git/javadoc/com/google/inject/Guice.html#createInjector(java.lang.Iterable<?
> extends com.google.inject.Module>)
>
> http://people.apache.org/~simonetripodi/
> http://simonetripodi.livejournal.com/
> http://twitter.com/simonetripodi
> http://www.99soft.org/
Re: [lifecycle] polishing org.apache.onami.lifecycle.core.ListBuilder
Posted by Simone Tripodi <si...@apache.org>.
> Yeah - the big problem is that this generates a warning:
>
> Arrays.asList( WarmUp.class, PostConstruct.class )
>
personally I am not so worried about that warnings, they just make
some noise - feel free to include them or not.
I'd suggest anyway to support Iterable<E> rather than just List<E> for
accepting annotation types, like Guice.createInjector()[1] does
> I'm glad that this is fixed in Java 7. BTW - why are we targeting Java 5? Can we at least move to Java 6?
do you need some JVM feature that require JDK6? if yes - please
specify the reasons - you can override the ${javac.src.version} and
${javac.target.version} properties in the component pom.
if not, ... which is the sense to upgrade to 6? :) having APIs still
be backward compatible with JDK5 is still fine since Guice targets it
- and immagine I still have
customers which run their application on JDK1.4! :D
best,
-Simo
[1] http://google-guice.googlecode.com/git/javadoc/com/google/inject/Guice.html#createInjector(java.lang.Iterable<?
extends com.google.inject.Module>)
http://people.apache.org/~simonetripodi/
http://simonetripodi.livejournal.com/
http://twitter.com/simonetripodi
http://www.99soft.org/
Re: [lifecycle] polishing org.apache.onami.lifecycle.core.ListBuilder
Posted by Jordan Zimmerman <jo...@jordanzimmerman.com>.
Yeah - the big problem is that this generates a warning:
Arrays.asList( WarmUp.class, PostConstruct.class )
I'm glad that this is fixed in Java 7. BTW - why are we targeting Java 5? Can we at least move to Java 6?
-JZ
On Mar 13, 2013, at 7:54 PM, Mikhail Mazursky <mi...@gmail.com> wrote:
> Hi Simone.
>
> I little inconvenience with varargs is that it will produce warnings is
> user code because we use parametrized type Class<? extends Annotation> in
> it (see [1]). The correct solution would be to use @SafeVarargs [2] but it
> was only added in Java 7 and we target Java 5. So i think we better keep
> current constructor intact. When i mentioned asList() i was talking about
> internal use of it in Onami code instead of ListBuilder.
>
> Also Onami user can use asList() himself and it is annotated with
> @SafeVarargs in Java 7+ so no warnings in this case.
>
> WDYT?
>
> [1]: http://stackoverflow.com/questions/4257883/warning-for-generic-varargs
> [2]: http://docs.oracle.com/javase/7/docs/api/java/lang/SafeVarargs.html
>
> 2013/3/14 Simone Tripodi <si...@apache.org>
>
>> Hi Mikhail!
>>
>> +1 for varargs, result would look charming like a sunshine:
>>
>> public AbstractLifeCycleModule( Class<? extends
>> Annotation>...annotationTypes )
>> {
>> this( any(), asList( annotationTypes ) );
>> }
>>
>> Of course I'd change the overloading constructor as well:
>>
>> public AbstractLifeCycleModule( Matcher<? super TypeLiteral<?>>
>> typeMatcher, Class<? extends Annotation>...annotationTypes )
>> {
>> this( Arrays.asList( annotationTypes ), typeMatcher );
>> }
>>
>> Clean, compact, clear, direct. We cannot ask more :)
>> Feel free to fill an issue and provide a fix.
>>
>> my 2 cents,
>> -Simo
>>
>> http://people.apache.org/~simonetripodi/
>> http://simonetripodi.livejournal.com/
>> http://twitter.com/simonetripodi
>> http://www.99soft.org/
>>
>>
>> On Wed, Mar 13, 2013 at 1:15 PM, Mikhail Mazursky
>> <mi...@gmail.com> wrote:
>>> Hi Simone, Jordan.
>>>
>>> IMHO AbstractLifeCycleModule should check that the List that was passed
>> to
>>> it contains no duplicates and throw exception if it does. In any case
>>> behaviour in such situation should be described it JavaDocs.
>>> Also, is this ListBuilder need to be public API at all? And maybe get rid
>>> of it completely by using Arrays.asList()?
>>>
>>> 2013/3/12 Simone Tripodi <si...@apache.org>
>>>
>>>> Hi Jordan,
>>>>
>>>>>> * the adapted List of annotation should be IMHO a LinkedHashSet,
>>>>>> otherwise users could specify the same type twice in the list and
>>>>>> drive the lifecycle engine to invoke twice the same method in
>>>>>> different phases - IIUC this is not the desired behaviour, or is it?
>>>>> It's a List because it needs to be ordered. I hadn't thought about
>>>> duplicates.
>>>>
>>>> the LinkedHashSet[1] will preserve the order like the List does,
>>>> moreover will keep the unicity of its elements.
>>>>
>>>>
>>
Re: [lifecycle] polishing org.apache.onami.lifecycle.core.ListBuilder
Posted by Mikhail Mazursky <mi...@gmail.com>.
Hi Simone.
I little inconvenience with varargs is that it will produce warnings is
user code because we use parametrized type Class<? extends Annotation> in
it (see [1]). The correct solution would be to use @SafeVarargs [2] but it
was only added in Java 7 and we target Java 5. So i think we better keep
current constructor intact. When i mentioned asList() i was talking about
internal use of it in Onami code instead of ListBuilder.
Also Onami user can use asList() himself and it is annotated with
@SafeVarargs in Java 7+ so no warnings in this case.
WDYT?
[1]: http://stackoverflow.com/questions/4257883/warning-for-generic-varargs
[2]: http://docs.oracle.com/javase/7/docs/api/java/lang/SafeVarargs.html
2013/3/14 Simone Tripodi <si...@apache.org>
> Hi Mikhail!
>
> +1 for varargs, result would look charming like a sunshine:
>
> public AbstractLifeCycleModule( Class<? extends
> Annotation>...annotationTypes )
> {
> this( any(), asList( annotationTypes ) );
> }
>
> Of course I'd change the overloading constructor as well:
>
> public AbstractLifeCycleModule( Matcher<? super TypeLiteral<?>>
> typeMatcher, Class<? extends Annotation>...annotationTypes )
> {
> this( Arrays.asList( annotationTypes ), typeMatcher );
> }
>
> Clean, compact, clear, direct. We cannot ask more :)
> Feel free to fill an issue and provide a fix.
>
> my 2 cents,
> -Simo
>
> http://people.apache.org/~simonetripodi/
> http://simonetripodi.livejournal.com/
> http://twitter.com/simonetripodi
> http://www.99soft.org/
>
>
> On Wed, Mar 13, 2013 at 1:15 PM, Mikhail Mazursky
> <mi...@gmail.com> wrote:
> > Hi Simone, Jordan.
> >
> > IMHO AbstractLifeCycleModule should check that the List that was passed
> to
> > it contains no duplicates and throw exception if it does. In any case
> > behaviour in such situation should be described it JavaDocs.
> > Also, is this ListBuilder need to be public API at all? And maybe get rid
> > of it completely by using Arrays.asList()?
> >
> > 2013/3/12 Simone Tripodi <si...@apache.org>
> >
> >> Hi Jordan,
> >>
> >> >> * the adapted List of annotation should be IMHO a LinkedHashSet,
> >> >> otherwise users could specify the same type twice in the list and
> >> >> drive the lifecycle engine to invoke twice the same method in
> >> >> different phases - IIUC this is not the desired behaviour, or is it?
> >> > It's a List because it needs to be ordered. I hadn't thought about
> >> duplicates.
> >>
> >> the LinkedHashSet[1] will preserve the order like the List does,
> >> moreover will keep the unicity of its elements.
> >>
> >>
>
Re: [lifecycle] polishing org.apache.onami.lifecycle.core.ListBuilder
Posted by Simone Tripodi <si...@apache.org>.
my previous samples contain errors, due to rush on replying - sorry :)
http://people.apache.org/~simonetripodi/
http://simonetripodi.livejournal.com/
http://twitter.com/simonetripodi
http://www.99soft.org/
On Wed, Mar 13, 2013 at 6:13 PM, Simone Tripodi
<si...@apache.org> wrote:
> Hi Mikhail!
>
> +1 for varargs, result would look charming like a sunshine:
>
> public AbstractLifeCycleModule( Class<? extends
> Annotation>...annotationTypes )
> {
> this( any(), asList( annotationTypes ) );
> }
>
> Of course I'd change the overloading constructor as well:
>
> public AbstractLifeCycleModule( Matcher<? super TypeLiteral<?>>
> typeMatcher, Class<? extends Annotation>...annotationTypes )
> {
> this( Arrays.asList( annotationTypes ), typeMatcher );
> }
>
> Clean, compact, clear, direct. We cannot ask more :)
> Feel free to fill an issue and provide a fix.
>
> my 2 cents,
> -Simo
>
> http://people.apache.org/~simonetripodi/
> http://simonetripodi.livejournal.com/
> http://twitter.com/simonetripodi
> http://www.99soft.org/
>
>
> On Wed, Mar 13, 2013 at 1:15 PM, Mikhail Mazursky
> <mi...@gmail.com> wrote:
>> Hi Simone, Jordan.
>>
>> IMHO AbstractLifeCycleModule should check that the List that was passed to
>> it contains no duplicates and throw exception if it does. In any case
>> behaviour in such situation should be described it JavaDocs.
>> Also, is this ListBuilder need to be public API at all? And maybe get rid
>> of it completely by using Arrays.asList()?
>>
>> 2013/3/12 Simone Tripodi <si...@apache.org>
>>
>>> Hi Jordan,
>>>
>>> >> * the adapted List of annotation should be IMHO a LinkedHashSet,
>>> >> otherwise users could specify the same type twice in the list and
>>> >> drive the lifecycle engine to invoke twice the same method in
>>> >> different phases - IIUC this is not the desired behaviour, or is it?
>>> > It's a List because it needs to be ordered. I hadn't thought about
>>> duplicates.
>>>
>>> the LinkedHashSet[1] will preserve the order like the List does,
>>> moreover will keep the unicity of its elements.
>>>
>>>
Re: [lifecycle] polishing org.apache.onami.lifecycle.core.ListBuilder
Posted by Simone Tripodi <si...@apache.org>.
Hi Mikhail!
+1 for varargs, result would look charming like a sunshine:
public AbstractLifeCycleModule( Class<? extends
Annotation>...annotationTypes )
{
this( any(), asList( annotationTypes ) );
}
Of course I'd change the overloading constructor as well:
public AbstractLifeCycleModule( Matcher<? super TypeLiteral<?>>
typeMatcher, Class<? extends Annotation>...annotationTypes )
{
this( Arrays.asList( annotationTypes ), typeMatcher );
}
Clean, compact, clear, direct. We cannot ask more :)
Feel free to fill an issue and provide a fix.
my 2 cents,
-Simo
http://people.apache.org/~simonetripodi/
http://simonetripodi.livejournal.com/
http://twitter.com/simonetripodi
http://www.99soft.org/
On Wed, Mar 13, 2013 at 1:15 PM, Mikhail Mazursky
<mi...@gmail.com> wrote:
> Hi Simone, Jordan.
>
> IMHO AbstractLifeCycleModule should check that the List that was passed to
> it contains no duplicates and throw exception if it does. In any case
> behaviour in such situation should be described it JavaDocs.
> Also, is this ListBuilder need to be public API at all? And maybe get rid
> of it completely by using Arrays.asList()?
>
> 2013/3/12 Simone Tripodi <si...@apache.org>
>
>> Hi Jordan,
>>
>> >> * the adapted List of annotation should be IMHO a LinkedHashSet,
>> >> otherwise users could specify the same type twice in the list and
>> >> drive the lifecycle engine to invoke twice the same method in
>> >> different phases - IIUC this is not the desired behaviour, or is it?
>> > It's a List because it needs to be ordered. I hadn't thought about
>> duplicates.
>>
>> the LinkedHashSet[1] will preserve the order like the List does,
>> moreover will keep the unicity of its elements.
>>
>>
Re: [lifecycle] polishing org.apache.onami.lifecycle.core.ListBuilder
Posted by Mikhail Mazursky <mi...@gmail.com>.
Hi Simone, Jordan.
IMHO AbstractLifeCycleModule should check that the List that was passed to
it contains no duplicates and throw exception if it does. In any case
behaviour in such situation should be described it JavaDocs.
Also, is this ListBuilder need to be public API at all? And maybe get rid
of it completely by using Arrays.asList()?
2013/3/12 Simone Tripodi <si...@apache.org>
> Hi Jordan,
>
> >> * the adapted List of annotation should be IMHO a LinkedHashSet,
> >> otherwise users could specify the same type twice in the list and
> >> drive the lifecycle engine to invoke twice the same method in
> >> different phases - IIUC this is not the desired behaviour, or is it?
> > It's a List because it needs to be ordered. I hadn't thought about
> duplicates.
>
> the LinkedHashSet[1] will preserve the order like the List does,
> moreover will keep the unicity of its elements.
>
>
Re: [lifecycle] polishing org.apache.onami.lifecycle.core.ListBuilder
Posted by Simone Tripodi <si...@apache.org>.
Hi Jordan,
>> * the adapted List of annotation should be IMHO a LinkedHashSet,
>> otherwise users could specify the same type twice in the list and
>> drive the lifecycle engine to invoke twice the same method in
>> different phases - IIUC this is not the desired behaviour, or is it?
> It's a List because it needs to be ordered. I hadn't thought about duplicates.
the LinkedHashSet[1] will preserve the order like the List does,
moreover will keep the unicity of its elements.
> If you open a ticket on this I'll make the changes.
sure, will do ASAP! :)
Best,
-Simo
[1] http://docs.oracle.com/javase/6/docs/api/java/util/LinkedHashSet.html
http://people.apache.org/~simonetripodi/
http://simonetripodi.livejournal.com/
http://twitter.com/simonetripodi
http://www.99soft.org/
On Mon, Mar 11, 2013 at 9:59 PM, Jordan Zimmerman
<jo...@jordanzimmerman.com> wrote:
>> * calling it ListBuilder is a little confusing, because the name
>> reminds me a collections general purpose class, I'd rename it to
>> LifecycleAnnotationsListBuilder;
> That's probably a better name. Ideally, I'd just use Guava but I didn't want to introduce another dependency.
>
>> * the adapted List of annotation should be IMHO a LinkedHashSet,
>> otherwise users could specify the same type twice in the list and
>> drive the lifecycle engine to invoke twice the same method in
>> different phases - IIUC this is not the desired behaviour, or is it?
> It's a List because it needs to be ordered. I hadn't thought about duplicates.
>
> Maybe the right thing to do is to turn this into a "first class" object instead of just a helper. Like you say in your third point, have LifeCycleModule only take LifecycleAnnotationsList. LifecycleAnnotationsListBuilder would build an ordered, unique list.
>
> If you open a ticket on this I'll make the changes.
>
> -JZ
>
> On Mar 11, 2013, at 1:18 PM, Simone Tripodi <si...@apache.org> wrote:
>
>> Hi Jordan/all,
>>
>> while reviewing the incredible work that Jordan just checked-in in
>> [lifecycle] module, I got interested by ListBuilder class, I have few
>> questions/observations:
>>
>> * calling it ListBuilder is a little confusing, because the name
>> reminds me a collections general purpose class, I'd rename it to
>> LifecycleAnnotationsListBuilder;
>>
>> * the adapted List of annotation should be IMHO a LinkedHashSet,
>> otherwise users could specify the same type twice in the list and
>> drive the lifecycle engine to invoke twice the same method in
>> different phases - IIUC this is not the desired behaviour, or is it?
>>
>> * according to previous point,
>> org.apache.onami.lifecycle.core.LifeCycleModule should be modified in
>> order to accept a more appropriate data structure.
>>
>> WDYT?
>> TIA, all the best,
>> -Simo
>>
>> http://people.apache.org/~simonetripodi/
>> http://simonetripodi.livejournal.com/
>> http://twitter.com/simonetripodi
>> http://www.99soft.org/
>
Re: [lifecycle] polishing org.apache.onami.lifecycle.core.ListBuilder
Posted by Jordan Zimmerman <jo...@jordanzimmerman.com>.
> * calling it ListBuilder is a little confusing, because the name
> reminds me a collections general purpose class, I'd rename it to
> LifecycleAnnotationsListBuilder;
That's probably a better name. Ideally, I'd just use Guava but I didn't want to introduce another dependency.
> * the adapted List of annotation should be IMHO a LinkedHashSet,
> otherwise users could specify the same type twice in the list and
> drive the lifecycle engine to invoke twice the same method in
> different phases - IIUC this is not the desired behaviour, or is it?
It's a List because it needs to be ordered. I hadn't thought about duplicates.
Maybe the right thing to do is to turn this into a "first class" object instead of just a helper. Like you say in your third point, have LifeCycleModule only take LifecycleAnnotationsList. LifecycleAnnotationsListBuilder would build an ordered, unique list.
If you open a ticket on this I'll make the changes.
-JZ
On Mar 11, 2013, at 1:18 PM, Simone Tripodi <si...@apache.org> wrote:
> Hi Jordan/all,
>
> while reviewing the incredible work that Jordan just checked-in in
> [lifecycle] module, I got interested by ListBuilder class, I have few
> questions/observations:
>
> * calling it ListBuilder is a little confusing, because the name
> reminds me a collections general purpose class, I'd rename it to
> LifecycleAnnotationsListBuilder;
>
> * the adapted List of annotation should be IMHO a LinkedHashSet,
> otherwise users could specify the same type twice in the list and
> drive the lifecycle engine to invoke twice the same method in
> different phases - IIUC this is not the desired behaviour, or is it?
>
> * according to previous point,
> org.apache.onami.lifecycle.core.LifeCycleModule should be modified in
> order to accept a more appropriate data structure.
>
> WDYT?
> TIA, all the best,
> -Simo
>
> http://people.apache.org/~simonetripodi/
> http://simonetripodi.livejournal.com/
> http://twitter.com/simonetripodi
> http://www.99soft.org/