You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Ignasi Barrera <na...@apache.org> on 2014/03/06 11:45:40 UTC
Varargs in options (bad) pattern
Hi!
This thread [1] brought onto the table a bad pattern that is widely used in
the project. Many methods in the APIs use a varargs parameter for the
additional options, just to make the parameter optional.
This isn't good, as it allows users to pass more than one option object,
which is not good and will fail at runtime. Instead, in those cases there
should be two methods, one with the options parameter, and one without it.
I've done a quick search to see how many methods we have in the APIs
following that bad pattern:
How many methods are there using the bad pattern:
nacx@maqui:~/src/asf $ for d in jclouds jclouds-chef jclouds-labs*; do echo
-n $d; grep -r '[oO]ptions\s*\.\.\.' $d | grep -v '/target/' | wc -l ; done
jclouds 224
jclouds-chef 0
jclouds-labs 9
jclouds-labs-aws 0
jclouds-labs-google 0
jclouds-labs-openstack 11
How many files:
nacx@maqui:~/src/asf $ for d in jclouds jclouds-chef jclouds-labs*; do echo
-n $d; grep -lr '[oO]ptions\s*\.\.\.' $d | grep -v '/target/' | wc -l ; done
jclouds 86
jclouds-chef 0
jclouds-labs 6
jclouds-labs-aws 0
jclouds-labs-google 0
jclouds-labs-openstack 5
As you see, in the main repo there are many of them. So, WDYT about:
1) Keeping the current pattern and continuing to use it
2) Keeping the current pattern where already present, but not adding new
instances of it
3) A PR to clean up occurrences of this pattern
...?
Ignasi
[1] http://markmail.org/message/ybooic67dtlt27hv
Re: Varargs in options (bad) pattern
Posted by Ignasi Barrera <ig...@gmail.com>.
If you can remove the pattern while unasynching I'd say go for it! :)
Removing technical debt is always welcome. If it introduces too much
overhead, however, I'd say go step by step and do it later in a different
PR.
El 10/04/2014 22:54, "Jeremy Daggett" <je...@rackspace.com>
escribió:
> Hey great! I appreciate the feedback.
>
> I also thought that was the original intention, but it didn¹t work out
> that way. As Andrew mentioned, passing a whole bunch of options is just
> plain silly! A single Options class should suffice for any API.
>
> With some of the unasyncing I am doing now around the OpenStack APIs, we
> could realistically remove them from any of those APIs that I touch. WDYT?
>
> Maybe we can eliminate this pattern completely in a future release...
>
> /jd
>
> On 4/10/14, 1:06 PM, "Ignasi Barrera" <ig...@gmail.com> wrote:
>
> >Thanks for bumping this up Jeremy!
> >
> >The problem with this option that we would allow users to do something
> >(passing more than one arg) that will fail 100% of the times.
> >
> >Overloading the method is almost immediate and effort-less, and having
> >methods with the same name will properly reflect that you can pass one and
> >only one option object.
> >
> >As it seems too much work for a realistic PR in the short term, I'd go for
> >option 2 and take care of not using that pattern anymore and eventually
> >fix
> >the ones we find when changing some api that uses the bad pattern.
> >
> >I.
> >El 10/04/2014 21:46, "Jeremy Daggett" <je...@gmail.com>
> escribió:
> >
> >> Picking back up on this topic...
> >>
> >> I am actually leaning towards option 1 now.
> >>
> >> One thing that is really handy with varargs is that rather than
> >>providing
> >> multiple methods, we can have a single method in an interface. It could
> >> really simplify the APIs for our users.
> >>
> >> For example, if an API has a no-arg "list()" method, we could express
> >>it as
> >> a single method in the interface "list(ListOptions...)". The cool thing
> >>is
> >> that we get the "list()" method inherently without specifying it
> >> explicitly. :)
> >>
> >> For that matter, we could potentially remove all of the "NONE" fields
> >>from
> >> the Options classes across the codebase.
> >>
> >> Throwing it out there for feedback, thanks!
> >>
> >> /jd
> >>
> >>
> >> On Fri, Mar 7, 2014 at 1:37 PM, Jeremy Daggett <
> jeremy.daggett@gmail.com
> >> >wrote:
> >>
> >> > Thanks nacx!
> >> >
> >> > I have spent a lot of time with the options classes recently, and I
> >>have
> >> > seen this pattern all over the place. I have never understood the
> >>intent
> >> of
> >> > using varargs for the options, and I suspect that nobody relies on
> >>them.
> >> >
> >> > My opinion is that we should clean them up and get rid of the funny
> >> smell.
> >> > ;)
> >> >
> >> > Does anyone know if there have been any bug reports on runtime
> >>failures?
> >> >
> >> > /jd
> >> >
> >> >
> >> > On Thu, Mar 6, 2014 at 2:45 AM, Ignasi Barrera <na...@apache.org>
> >>wrote:
> >> >
> >> >> Hi!
> >> >>
> >> >> This thread [1] brought onto the table a bad pattern that is widely
> >>used
> >> >> in
> >> >> the project. Many methods in the APIs use a varargs parameter for the
> >> >> additional options, just to make the parameter optional.
> >> >>
> >> >> This isn't good, as it allows users to pass more than one option
> >>object,
> >> >> which is not good and will fail at runtime. Instead, in those cases
> >> there
> >> >> should be two methods, one with the options parameter, and one
> >>without
> >> it.
> >> >>
> >> >> I've done a quick search to see how many methods we have in the APIs
> >> >> following that bad pattern:
> >> >>
> >> >> How many methods are there using the bad pattern:
> >> >>
> >> >> nacx@maqui:~/src/asf $ for d in jclouds jclouds-chef jclouds-labs*;
> >>do
> >> >> echo
> >> >> -n $d; grep -r '[oO]ptions\s*\.\.\.' $d | grep -v '/target/' | wc -l
> >>;
> >> >> done
> >> >> jclouds 224
> >> >> jclouds-chef 0
> >> >> jclouds-labs 9
> >> >> jclouds-labs-aws 0
> >> >> jclouds-labs-google 0
> >> >> jclouds-labs-openstack 11
> >> >>
> >> >> How many files:
> >> >>
> >> >> nacx@maqui:~/src/asf $ for d in jclouds jclouds-chef jclouds-labs*;
> >>do
> >> >> echo
> >> >> -n $d; grep -lr '[oO]ptions\s*\.\.\.' $d | grep -v '/target/' | wc
> >>-l ;
> >> >> done
> >> >> jclouds 86
> >> >> jclouds-chef 0
> >> >> jclouds-labs 6
> >> >> jclouds-labs-aws 0
> >> >> jclouds-labs-google 0
> >> >> jclouds-labs-openstack 5
> >> >>
> >> >> As you see, in the main repo there are many of them. So, WDYT about:
> >> >>
> >> >> 1) Keeping the current pattern and continuing to use it
> >> >> 2) Keeping the current pattern where already present, but not adding
> >>new
> >> >> instances of it
> >> >> 3) A PR to clean up occurrences of this pattern
> >> >>
> >> >> ...?
> >> >>
> >> >>
> >> >> Ignasi
> >> >>
> >> >>
> >> >> [1] http://markmail.org/message/ybooic67dtlt27hv
> >> >>
> >> >
> >> >
> >>
>
>
Re: Varargs in options (bad) pattern
Posted by Jeremy Daggett <je...@RACKSPACE.COM>.
Hey great! I appreciate the feedback.
I also thought that was the original intention, but it didn¹t work out
that way. As Andrew mentioned, passing a whole bunch of options is just
plain silly! A single Options class should suffice for any API.
With some of the unasyncing I am doing now around the OpenStack APIs, we
could realistically remove them from any of those APIs that I touch. WDYT?
Maybe we can eliminate this pattern completely in a future release...
/jd
On 4/10/14, 1:06 PM, "Ignasi Barrera" <ig...@gmail.com> wrote:
>Thanks for bumping this up Jeremy!
>
>The problem with this option that we would allow users to do something
>(passing more than one arg) that will fail 100% of the times.
>
>Overloading the method is almost immediate and effort-less, and having
>methods with the same name will properly reflect that you can pass one and
>only one option object.
>
>As it seems too much work for a realistic PR in the short term, I'd go for
>option 2 and take care of not using that pattern anymore and eventually
>fix
>the ones we find when changing some api that uses the bad pattern.
>
>I.
>El 10/04/2014 21:46, "Jeremy Daggett" <je...@gmail.com> escribió:
>
>> Picking back up on this topic...
>>
>> I am actually leaning towards option 1 now.
>>
>> One thing that is really handy with varargs is that rather than
>>providing
>> multiple methods, we can have a single method in an interface. It could
>> really simplify the APIs for our users.
>>
>> For example, if an API has a no-arg "list()" method, we could express
>>it as
>> a single method in the interface "list(ListOptions...)". The cool thing
>>is
>> that we get the "list()" method inherently without specifying it
>> explicitly. :)
>>
>> For that matter, we could potentially remove all of the "NONE" fields
>>from
>> the Options classes across the codebase.
>>
>> Throwing it out there for feedback, thanks!
>>
>> /jd
>>
>>
>> On Fri, Mar 7, 2014 at 1:37 PM, Jeremy Daggett <jeremy.daggett@gmail.com
>> >wrote:
>>
>> > Thanks nacx!
>> >
>> > I have spent a lot of time with the options classes recently, and I
>>have
>> > seen this pattern all over the place. I have never understood the
>>intent
>> of
>> > using varargs for the options, and I suspect that nobody relies on
>>them.
>> >
>> > My opinion is that we should clean them up and get rid of the funny
>> smell.
>> > ;)
>> >
>> > Does anyone know if there have been any bug reports on runtime
>>failures?
>> >
>> > /jd
>> >
>> >
>> > On Thu, Mar 6, 2014 at 2:45 AM, Ignasi Barrera <na...@apache.org>
>>wrote:
>> >
>> >> Hi!
>> >>
>> >> This thread [1] brought onto the table a bad pattern that is widely
>>used
>> >> in
>> >> the project. Many methods in the APIs use a varargs parameter for the
>> >> additional options, just to make the parameter optional.
>> >>
>> >> This isn't good, as it allows users to pass more than one option
>>object,
>> >> which is not good and will fail at runtime. Instead, in those cases
>> there
>> >> should be two methods, one with the options parameter, and one
>>without
>> it.
>> >>
>> >> I've done a quick search to see how many methods we have in the APIs
>> >> following that bad pattern:
>> >>
>> >> How many methods are there using the bad pattern:
>> >>
>> >> nacx@maqui:~/src/asf $ for d in jclouds jclouds-chef jclouds-labs*;
>>do
>> >> echo
>> >> -n $d; grep -r '[oO]ptions\s*\.\.\.' $d | grep -v '/target/' | wc -l
>>;
>> >> done
>> >> jclouds 224
>> >> jclouds-chef 0
>> >> jclouds-labs 9
>> >> jclouds-labs-aws 0
>> >> jclouds-labs-google 0
>> >> jclouds-labs-openstack 11
>> >>
>> >> How many files:
>> >>
>> >> nacx@maqui:~/src/asf $ for d in jclouds jclouds-chef jclouds-labs*;
>>do
>> >> echo
>> >> -n $d; grep -lr '[oO]ptions\s*\.\.\.' $d | grep -v '/target/' | wc
>>-l ;
>> >> done
>> >> jclouds 86
>> >> jclouds-chef 0
>> >> jclouds-labs 6
>> >> jclouds-labs-aws 0
>> >> jclouds-labs-google 0
>> >> jclouds-labs-openstack 5
>> >>
>> >> As you see, in the main repo there are many of them. So, WDYT about:
>> >>
>> >> 1) Keeping the current pattern and continuing to use it
>> >> 2) Keeping the current pattern where already present, but not adding
>>new
>> >> instances of it
>> >> 3) A PR to clean up occurrences of this pattern
>> >>
>> >> ...?
>> >>
>> >>
>> >> Ignasi
>> >>
>> >>
>> >> [1] http://markmail.org/message/ybooic67dtlt27hv
>> >>
>> >
>> >
>>
Re: Varargs in options (bad) pattern
Posted by Ignasi Barrera <ig...@gmail.com>.
Thanks for bumping this up Jeremy!
The problem with this option that we would allow users to do something
(passing more than one arg) that will fail 100% of the times.
Overloading the method is almost immediate and effort-less, and having
methods with the same name will properly reflect that you can pass one and
only one option object.
As it seems too much work for a realistic PR in the short term, I'd go for
option 2 and take care of not using that pattern anymore and eventually fix
the ones we find when changing some api that uses the bad pattern.
I.
El 10/04/2014 21:46, "Jeremy Daggett" <je...@gmail.com> escribió:
> Picking back up on this topic...
>
> I am actually leaning towards option 1 now.
>
> One thing that is really handy with varargs is that rather than providing
> multiple methods, we can have a single method in an interface. It could
> really simplify the APIs for our users.
>
> For example, if an API has a no-arg "list()" method, we could express it as
> a single method in the interface "list(ListOptions...)". The cool thing is
> that we get the "list()" method inherently without specifying it
> explicitly. :)
>
> For that matter, we could potentially remove all of the "NONE" fields from
> the Options classes across the codebase.
>
> Throwing it out there for feedback, thanks!
>
> /jd
>
>
> On Fri, Mar 7, 2014 at 1:37 PM, Jeremy Daggett <jeremy.daggett@gmail.com
> >wrote:
>
> > Thanks nacx!
> >
> > I have spent a lot of time with the options classes recently, and I have
> > seen this pattern all over the place. I have never understood the intent
> of
> > using varargs for the options, and I suspect that nobody relies on them.
> >
> > My opinion is that we should clean them up and get rid of the funny
> smell.
> > ;)
> >
> > Does anyone know if there have been any bug reports on runtime failures?
> >
> > /jd
> >
> >
> > On Thu, Mar 6, 2014 at 2:45 AM, Ignasi Barrera <na...@apache.org> wrote:
> >
> >> Hi!
> >>
> >> This thread [1] brought onto the table a bad pattern that is widely used
> >> in
> >> the project. Many methods in the APIs use a varargs parameter for the
> >> additional options, just to make the parameter optional.
> >>
> >> This isn't good, as it allows users to pass more than one option object,
> >> which is not good and will fail at runtime. Instead, in those cases
> there
> >> should be two methods, one with the options parameter, and one without
> it.
> >>
> >> I've done a quick search to see how many methods we have in the APIs
> >> following that bad pattern:
> >>
> >> How many methods are there using the bad pattern:
> >>
> >> nacx@maqui:~/src/asf $ for d in jclouds jclouds-chef jclouds-labs*; do
> >> echo
> >> -n $d; grep -r '[oO]ptions\s*\.\.\.' $d | grep -v '/target/' | wc -l ;
> >> done
> >> jclouds 224
> >> jclouds-chef 0
> >> jclouds-labs 9
> >> jclouds-labs-aws 0
> >> jclouds-labs-google 0
> >> jclouds-labs-openstack 11
> >>
> >> How many files:
> >>
> >> nacx@maqui:~/src/asf $ for d in jclouds jclouds-chef jclouds-labs*; do
> >> echo
> >> -n $d; grep -lr '[oO]ptions\s*\.\.\.' $d | grep -v '/target/' | wc -l ;
> >> done
> >> jclouds 86
> >> jclouds-chef 0
> >> jclouds-labs 6
> >> jclouds-labs-aws 0
> >> jclouds-labs-google 0
> >> jclouds-labs-openstack 5
> >>
> >> As you see, in the main repo there are many of them. So, WDYT about:
> >>
> >> 1) Keeping the current pattern and continuing to use it
> >> 2) Keeping the current pattern where already present, but not adding new
> >> instances of it
> >> 3) A PR to clean up occurrences of this pattern
> >>
> >> ...?
> >>
> >>
> >> Ignasi
> >>
> >>
> >> [1] http://markmail.org/message/ybooic67dtlt27hv
> >>
> >
> >
>
Re: Varargs in options (bad) pattern
Posted by Andrew Phillips <ap...@qrmedia.com>.
Quoting Jeremy Daggett <je...@gmail.com>:
> Picking back up on this topic...
>
> I am actually leaning towards option 1 now.
>
> One thing that is really handy with varargs is that rather than providing
> multiple methods, we can have a single method in an interface. It could
> really simplify the APIs for our users.
>
> For example, if an API has a no-arg "list()" method, we could express it as
> a single method in the interface "list(ListOptions...)".
...which, I think, was exactly the original intention. But it leaves
us unintentionally allowing you to call "list(ListOptions,
ListOptions, ListOptions, ...)", which simply makes no sense.
How much simplification for the users do you think this would be, by
the way? There would be no *code* changes required for users, just one
fewer Javadoc method to parse. That's nice, of course, but if one of
the methods simply refers directly to the other in the Javadoc, I hope
people won't have trouble finding the right one.
It *would* certainly allow us to cut down on method definition when
creating APIs, but personally I tend to agree with Ignasi in that I'd
rather make implementers define an extra method than make it possible
for users to make nonsensical API calls.
Perhaps we could cut down on the annotation overhead for implementers
by "inheriting" the annotations from the overloaded method?
ap
Re: Varargs in options (bad) pattern
Posted by Jeremy Daggett <je...@gmail.com>.
Picking back up on this topic...
I am actually leaning towards option 1 now.
One thing that is really handy with varargs is that rather than providing
multiple methods, we can have a single method in an interface. It could
really simplify the APIs for our users.
For example, if an API has a no-arg "list()" method, we could express it as
a single method in the interface "list(ListOptions...)". The cool thing is
that we get the "list()" method inherently without specifying it
explicitly. :)
For that matter, we could potentially remove all of the "NONE" fields from
the Options classes across the codebase.
Throwing it out there for feedback, thanks!
/jd
On Fri, Mar 7, 2014 at 1:37 PM, Jeremy Daggett <je...@gmail.com>wrote:
> Thanks nacx!
>
> I have spent a lot of time with the options classes recently, and I have
> seen this pattern all over the place. I have never understood the intent of
> using varargs for the options, and I suspect that nobody relies on them.
>
> My opinion is that we should clean them up and get rid of the funny smell.
> ;)
>
> Does anyone know if there have been any bug reports on runtime failures?
>
> /jd
>
>
> On Thu, Mar 6, 2014 at 2:45 AM, Ignasi Barrera <na...@apache.org> wrote:
>
>> Hi!
>>
>> This thread [1] brought onto the table a bad pattern that is widely used
>> in
>> the project. Many methods in the APIs use a varargs parameter for the
>> additional options, just to make the parameter optional.
>>
>> This isn't good, as it allows users to pass more than one option object,
>> which is not good and will fail at runtime. Instead, in those cases there
>> should be two methods, one with the options parameter, and one without it.
>>
>> I've done a quick search to see how many methods we have in the APIs
>> following that bad pattern:
>>
>> How many methods are there using the bad pattern:
>>
>> nacx@maqui:~/src/asf $ for d in jclouds jclouds-chef jclouds-labs*; do
>> echo
>> -n $d; grep -r '[oO]ptions\s*\.\.\.' $d | grep -v '/target/' | wc -l ;
>> done
>> jclouds 224
>> jclouds-chef 0
>> jclouds-labs 9
>> jclouds-labs-aws 0
>> jclouds-labs-google 0
>> jclouds-labs-openstack 11
>>
>> How many files:
>>
>> nacx@maqui:~/src/asf $ for d in jclouds jclouds-chef jclouds-labs*; do
>> echo
>> -n $d; grep -lr '[oO]ptions\s*\.\.\.' $d | grep -v '/target/' | wc -l ;
>> done
>> jclouds 86
>> jclouds-chef 0
>> jclouds-labs 6
>> jclouds-labs-aws 0
>> jclouds-labs-google 0
>> jclouds-labs-openstack 5
>>
>> As you see, in the main repo there are many of them. So, WDYT about:
>>
>> 1) Keeping the current pattern and continuing to use it
>> 2) Keeping the current pattern where already present, but not adding new
>> instances of it
>> 3) A PR to clean up occurrences of this pattern
>>
>> ...?
>>
>>
>> Ignasi
>>
>>
>> [1] http://markmail.org/message/ybooic67dtlt27hv
>>
>
>
Re: Varargs in options (bad) pattern
Posted by Jeremy Daggett <je...@gmail.com>.
Thanks nacx!
I have spent a lot of time with the options classes recently, and I have
seen this pattern all over the place. I have never understood the intent of
using varargs for the options, and I suspect that nobody relies on them.
My opinion is that we should clean them up and get rid of the funny smell.
;)
Does anyone know if there have been any bug reports on runtime failures?
/jd
On Thu, Mar 6, 2014 at 2:45 AM, Ignasi Barrera <na...@apache.org> wrote:
> Hi!
>
> This thread [1] brought onto the table a bad pattern that is widely used in
> the project. Many methods in the APIs use a varargs parameter for the
> additional options, just to make the parameter optional.
>
> This isn't good, as it allows users to pass more than one option object,
> which is not good and will fail at runtime. Instead, in those cases there
> should be two methods, one with the options parameter, and one without it.
>
> I've done a quick search to see how many methods we have in the APIs
> following that bad pattern:
>
> How many methods are there using the bad pattern:
>
> nacx@maqui:~/src/asf $ for d in jclouds jclouds-chef jclouds-labs*; do
> echo
> -n $d; grep -r '[oO]ptions\s*\.\.\.' $d | grep -v '/target/' | wc -l ; done
> jclouds 224
> jclouds-chef 0
> jclouds-labs 9
> jclouds-labs-aws 0
> jclouds-labs-google 0
> jclouds-labs-openstack 11
>
> How many files:
>
> nacx@maqui:~/src/asf $ for d in jclouds jclouds-chef jclouds-labs*; do
> echo
> -n $d; grep -lr '[oO]ptions\s*\.\.\.' $d | grep -v '/target/' | wc -l ;
> done
> jclouds 86
> jclouds-chef 0
> jclouds-labs 6
> jclouds-labs-aws 0
> jclouds-labs-google 0
> jclouds-labs-openstack 5
>
> As you see, in the main repo there are many of them. So, WDYT about:
>
> 1) Keeping the current pattern and continuing to use it
> 2) Keeping the current pattern where already present, but not adding new
> instances of it
> 3) A PR to clean up occurrences of this pattern
>
> ...?
>
>
> Ignasi
>
>
> [1] http://markmail.org/message/ybooic67dtlt27hv
>