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 <[email protected]>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 <[email protected]> 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 >> > >
