Code review comment for lp:~nuclearbob/utah/consolidate-scripts

On 04/25/2013 05:39 PM, Max Brustkern wrote:
>>> + else:
>>> > >+ kw = {'clean': (not args.no_destroy),
>>> > >+ 'new': True,
>>> > >+ }
>>> > >+ for arg in ['arch',
>>> > >+ 'boot',
>>> > >+ 'debug',
>>> > >+ 'image',
>>> > >+ 'initrd',
>>> > >+ 'kernel',
>>> > >+ 'name',
>>> > >+ 'preseed',
>>> > >+ 'rewrite',
>>> > >+ 'series',
>>> > >+ ]:
>>> > >+ kw[arg] = getattr(args, arg)
>> >
>> >this can be done explicityline and save two lines of code doing
>> >something like:
>> >
>> > kw['arch'] = args.arch
>> >
>> >and i think it might look more readable.
>> >
> I'm interested in what everybody else thinks of this, since I find the repetition very unpleasant to look at. Every time I see it, I think "we could just do that to a whole list of things and then add things to the list and I'll never make an error copying and pasting the code."

Two things:

1) fair enough - this is mostly subjective so we should decide as a team

2) I think this is actually more of a symptom of a larger problem.
Having a constructor that takes in 18 parameters is a bit extreme. Its
possible what I'm going to say is beyond the scope of this MP, but think
about the two cases we are using the "attr" hacks:

a) to set parameters in the call to the constructor
b) to take the parameters from the constructor and make them member
variables.

Now also remember the fact that we default things not provided to args
with a value from config.py.

I can't help but wonder if we shouldn't just pass "args" to the
constructor instead. I don't really like the idea of using "args", but
it feels cleaner than insane constructors.

Doing that would clean up my complain about run_utah_tests.py. You could
go extreme and then just assign self.args = args in Machine.__init__.
That feels a little crazy - and would probably turn into a fairly
big/error-prone refactor.