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)

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