Python exception-raising generator function

Question

I want my generator function to raise as soon as it can. If I make it yield the elements directly, then the KeyError won't happen until iteration begins. Is the following code the best I can do?

def cluster_terminals(self, key):
 """
 Deals with forwarding.
 """
 return_list = self.cluster_map[key]
 def internal_cluster_terminals(return_list):
 for x in return_list:
 if isinstance(x, ClusterTerminal):
 yield x
 elif isinstance(x, NodeTerminalMappingName):
 yield from x.node_terminal.cluster_terminals(x.mapping_name)
 else:
 assert False
 return internal_cluster_terminals(return_list)

• 200_success: Could you provide more background information about the task being performed? What class does this method belong to?
• Neil G: @200_success Thanks for taking a look. I just want to know about whether this is the right pattern to use for generators that raise immediately. I appreciate your interest in wanting to see the whole class, but I think it will take more time for me to explain it than the benefit of having it code reviewed?
• Dan Oberlam: So you want it to raise the error before you've started iteration?
• Dan Oberlam: Oh, I see what you mean now. I misunderstood before, carry on :P
• Eric: This still won't raise immediately if the yield from iterator decides to raise

Answer 1

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 returned). 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

• Neil G: This is excellent, thanks. I always forget how closed-over values work.