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

Revision history for this message
Max Brustkern (nuclearbob) wrote :

> On 04/25/2013 05:39 PM, Max Brustkern wrote:
> ...
> Two things:
>
> 1) fair enough - this is mostly subjective so we should decide as a team
>
Looks like your idea is preferred, so I implemented that.
> 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.
I think this larger problem is better solved by separating installation, and probably coming up with a completely new interface. I think as part of that sorting out which arguments are used for installation, machine management, and test running might serve us well and allow us to pass those around in ways that make more sense.

« Back to merge proposal