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