jclouds-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ignasi Barrera <ignasi.barr...@gmail.com>
Subject Re: Varargs in options (bad) pattern
Date Thu, 10 Apr 2014 20:56:55 GMT
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" <jeremy.daggett@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" <ignasi.barrera@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" <jeremy.daggett@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 <nacx@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
> >> >>
> >> >
> >> >
> >>
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message