google / openhtf

The open-source hardware testing framework.
Apache License 2.0
532 stars 218 forks source link

Reconsider flattening iterables of phases passed as args to Test.__init__() #199

Closed grybmadsci closed 2 years ago

grybmadsci commented 8 years ago

Some context from #196

The addition of parameterized phases as a concept is particularly appealing to me because it's a thing that I do in almost every test I've written, and I believe @wallacbe does that sort of thing a lot, too. However, as per the project philosophy, we do tend to be very conservative about the addition of new concepts like this, hence my wanting to get at least @jettisonjoe to weigh in before merging.

I think the addition of this concept alters the context for flattening iterables of phases, because such flattening is an extension of this concept in the sense that it's likely the primary use for parameterized phases will be looping over some input value. This puts us in a position to establish the convention of "when using .WithArgs() mapped over a list of values, do this, otherwise generally don't," whereas before we had no clear reason to tell users when to use Test(PhaseOne, PhaseTwo) vs Test([PhaseOne, PhaseTwo]), and that ambiguity was the primary reason for the pushback the first time.

What do people think? The tradeoff is slightly improved syntax in the .WIthArgs() loop case, at the expense of allowing the user to do silly things even outside that case:

test = Test(
  SetupPhase,
  (ParamPhase.WithArgs(p) for p in parameter_values),
  OtherPhase,
  (OtherParamPhase.WithArgs(o) for o in other_values))

vs.

phases = [SetupPhase]
phases.extend(ParamPhase.WithArgs(p) for p in parameter_values)
phases.append(OtherPhase)
phases.extend(OtherParamPhase.WithArgs(o) for o in other_values)
test = Test(*phases)

I think the first one is kind of nice, but I may be understimating how hard it is to understand without a Python background. Do people think it would be unnecessarily confusing and we should stick to the more vanilla second approach?

jettisonjoe commented 8 years ago

My original hesitation about this idea when it was raised was coming from the desire to avoid adding concepts/features to the framework without clear use cases. I agree that the addition of WithArgs() opens a nice opportunity for synergy/clarity here and also exposes a set of use cases that weren't as concrete or apparent before (at least not to me).

I'd also like to reiterate that my original response was a statement of reservations rather than a flat-out refusal. I'm convinced that there's more than enough justification here to seriously consider adding this automatic flattening.

glados-verma commented 2 years ago

This was implemented at some point. PhaseSequence, which is used by Test, accepts "Sequence of phase nodes, phase callables, or recursive iterables of either" and flattens them.