In terms of raising the KeyError
as soon cluster_terminals
is called (rather than when its result is iterated), this general pattern is likely the best. There are a couple of other points that bear mentioning, though.
First, assert False
is one of the worst ways to handle a detected error (marginally better than, say, sys.exit(0)
). Make the assertion self-documenting so that if when it gets triggered sometime in the future, you have more immediate information in the stack trace about what is going on. At a minimum, include a message with the assertion:
assert False, "`return_list` contains unexpected object of type " + str(type(x))
An arguably better option is to put it up front with an explicit test, even if that means duplicating some of your logic:
assert all(isinstance(x, (ClusterTerminal, NodeTerminalMappingName)) for x in return_list)
And then adjust your conditional to either have a pass
(and a short comment pointing to the assertion) in that branch, or omit that branch entirely. This makes it abundantly explicit to anyone reading your code that that is an 'impossible' situation, and means the invariants of this class have been broken (and that it isn't that client code has passed in bad data - for that, use a normal exception rather than an assertion).
A more minor quibble is that you don't need to pass return_list
into the helper function as an arguments. You can use closures:
return_list = self.cluster_map[key]
def internal_cluster_terminals():
...
return internal_cluster_terminals()
This will behave identically - Python functions can see variables from the scope they are defined in, even if that scope no longer 'exists' when they are executed.
internal_cluster_terminals
is a bit of an unwieldy name, and doesn't tell us anything we don't already know (it's.. internal to the function it's defined in. Wow, amazing). A more usual name for this type of function is something like helper
. return_list
isn't a great name either, especially since it's actually misleading (it isn't ever return
ed). Since it's a value from a mapping, call it after what those values conceptually are - cluster
might be a good option.
Finally, consider adding a short comment to document why you need a helper function in the first place, like:
# Use an internal generator instead of yielding directly
# so we can bail out early if there's a KeyError
yield from
iterator decides to raise \$\endgroup\$ – Eric Jul 27 '15 at 21:44