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