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