Fail Better

The Repo on GitHub

We’ll continue on our quest to do better Python. Today, we will try a match/case using a dictionary. It might make for better parsing of the tiny language that Bots and World share.

Last night I was browsing in Fluent Python and elsewhere, and I learned that the Python match/case can work with a dictionary in the match. That seems potentially useful, since we are dealing with a list of dictionaries in our action code. Here’s what we have now:

class World:
    def execute(self, actions_list):
        self.ids_used = set()
        self.execute_actions(actions_list)
        return [ self.fetch(bot_id) for bot_id in self.ids_used ]

    def execute_actions(self, actions_list):
        for action in actions_list:
            self.unpack_and_execute(**action)

    def unpack_and_execute(self, entity, verb, **parameters):
        if entity:
            self.ids_used.add(entity)
            entity_object = self.entity_from_id(entity)
        else:
            entity_object = None
        self.execute_action(entity_object, verb, parameters)

    def execute_action(self, entity, verb, parameters):
        match verb:
            case 'add_bot':
                self.add_bot_action(**parameters)
            case 'drop':
                self.drop_forward_action(entity, **parameters)
            case 'step':
                self.step(entity)
            case 'take':
                self.take_forward(entity)
            case 'turn':
                self.turn(entity, parameters['direction'])
            case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
                self.turn(entity, direction)
            case _:
                raise Exception(f'Unknown action {verb}')

I have no experience with using a dictionary in match/case, so I’ll try a test. But I do have a sense of where we’ll be going, so let me share that. We always expect an ‘entity’ key in every action. The value is the id of the bot in question, or zero if the verb is ‘add_bot’, since World assigns the ids. Now, if I’m not mistaken, cases might look like this:

case {'entity': 0, 'verb': 'add_bot'}
    ...
case {'entity': bot_id, 'verb': 'drop', 'holding': held_id}

In the first case, if I’m right, we can ask for a specific match, ‘entity’ zero and ‘verb’ ‘add_bot’. In the second, we still expect an ‘entity’ key, but we do not specify a value, just a name. If this case matches, the provided names will get the values from the specific entry being matched. If I’m right, that should allow us to have much more precise “parsing” of the commands, probably in less code.

We’ll have to try a few things to decide what’s best. It would be a shame, for example, if we had to translate the entity from id to WorldEntity in every case: we like to do things Once and Only Once.

Let’s do some tests to begin to understand this baby.

    def test_match_case_dict(self):
        d = {'entity': 101, 'verb': 'take'}
        match d:
            case {'entity': entity_id, 'verb': verb}:
                assert entity_id == 101
                assert verb == 'take'
            case _:
                assert False  # should never get here

That passes. And it fails if I tweak the values. Here are two more tests:

    def test_match_case_literal(self):
        d = {'entity': 0, 'verb': 'add_bot'}
        match d:
            case {'entity': 0, 'verb': 'add_bot'}:
                pass
            case _:
                assert False  # should never get here

    def test_match_case_literal_unmatched(self):
        d = {'entity': 101, 'verb': 'add_bot'}
        match d:
            case {'entity': 0, 'verb': 'add_bot'}:
                assert False  # should not match
            case { 'entity': entity_id, 'verb': 'take'}:
                assert False  # not here
            case { 'entity': entity_id, 'verb': verb}:
                assert entity_id == 101
                assert verb == 'add_bot'
            case _:
                assert False  # should match above

Those are passing also. Is there a way to flag an error in pytest? Might be able to replace the assert False with something better. A quick search doesn’t turn anything up. We’ll move on: these tests have given me enough confidence to go ahead with some improvements.

I expect to try more than one thing here. That suggests continuing with tests a bit longer: that way I won’t have to undo so many things in World. Back to the test file.

One idea is that we might pull the entity id first and plug back in an actual entity, like this:

    def test_plug_in_entity_object(self):
        def entity_from_id(entity_id):
            return f'entity_object_{entity_id}'
        d = {'entity': 0, 'verb': 'add_bot'}
        self.check_cases(d, entity_from_id)
        d = {'entity': 101, 'verb': 'take'}
        self.check_cases(d, entity_from_id)

    def check_cases(self, d, entity_from_id):
        match d:
            case {'entity': 0}:
                d['entity_object'] = None
            case {'entity': entity_id}:
                d['entity_object'] = entity_from_id(entity_id)
        match d:
            case {'entity_object': None, 'verb': 'add_bot'}:
                pass
            case {'entity_object': obj, 'verb': verb}:
                assert verb == 'take'
                assert obj == 'entity_object_101'
            case _:
                assert False

In the check_cases function, we add a new key, ‘entity_object’, with an identifiable string value, if the input key is non zero. Otherwise we provide None. Then we do our actual cases, but we know that we have the entity object available.

I don’t think we’ll do it quite that way in real life. We have no need for the None, we can parse, detect only the zero and ‘add_bot’, because we don’t need the zero.

I think I’ll be fumbling a bit, but I want to work in the real space now.

I transform this:

    def unpack_and_execute(self, entity, verb, **parameters):
        if entity:
            self.ids_used.add(entity)
            entity_object = self.entity_from_id(entity)
        else:
            entity_object = None
        self.execute_action(entity_object, verb, parameters)

To this:

    def execute_actions(self, actions_list):
        for action in actions_list:
            self.unpack_and_execute(**action)

    def unpack_and_execute(self, entity, **parameters):
        if entity:
            self.ids_used.add(entity)
            parameters['entity_object'] = self.entity_from_id(entity)
        self.execute_action(**parameters)

As written, this will leave parameters with a new key, ‘entity_object’, and without the ‘entity’ key. Ten tests break, no surprise there.

We need to fix execute_action, which starts like this:

    def execute_action(self, entity, verb, parameters):
        match verb:
            case 'add_bot':
                self.add_bot_action(**parameters)
            case 'drop':
                self.drop_forward_action(entity, **parameters)
            case 'step':
                self.step(entity)
            case 'take':
                self.take_forward(entity)
            case 'turn':
                self.turn(entity, parameters['direction'])
            case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
                self.turn(entity, direction)
            case _:
                raise Exception(f'Unknown action {verb}')

I’ll just change this to match our expectations. It does seem that I can take a smaller step than I anticipated and get to green.

    def unpack_and_execute(self, entity, **parameters):
        if entity:
            self.ids_used.add(entity)
            parameters['entity_object'] = self.entity_from_id(entity)
        self.execute_action(**parameters)

    def execute_action(self, entity_object, verb, **parameters):
        match verb:
            case 'add_bot':
                self.add_bot_action(**parameters)
            case 'drop':
                self.drop_forward_action(entity_object, **parameters)
            case 'step':
                self.step(entity_object)
            case 'take':
                self.take_forward(entity_object)
            case 'turn':
                self.turn(entity_object, parameters['direction'])
            case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
                self.turn(entity_object, direction)
            case _:
                raise Exception(f'Unknown action {verb}')

I’m down to one failure now.

    def test_add_bot(self):
        WorldEntity.next_id = 100
        world = World(10, 10)
        command = {'entity': 0,
                   'verb': 'add_bot',
                   'x': 5,
                   'y': 6,
                   'direction': 'EAST'}
        rq = [ command ]
        result = world.execute(rq)
        assert len(result) == 1

The objection is:

>       self.execute_action(**parameters)
E       TypeError: World.execute_action() missing 1 required positional argument: 'entity_object'

Ah. I’d better insert the None for now, even though no one will use it.

    def unpack_and_execute(self, entity, **parameters):
        if entity:
            self.ids_used.add(entity)
            parameters['entity_object'] = self.entity_from_id(entity)
        else:
            parameters['entity_object'] = None
        self.execute_action(**parameters)

We’re green. I am not ready to commit: this still might be a bad idea. Our mission now is to convert this:

    def execute_action(self, entity_object, verb, **parameters):
        match verb:
            case 'add_bot':
                self.add_bot_action(**parameters)
            case 'drop':
                self.drop_forward_action(entity_object, **parameters)
            case 'step':
                self.step(entity_object)
            case 'take':
                self.take_forward(entity_object)
            case 'turn':
                self.turn(entity_object, parameters['direction'])
            case 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction:
                self.turn(entity_object, direction)
            case _:
                raise Exception(f'Unknown action {verb}')

We want to pass in just the naked parameter dictionary and parse it in the cases. This will have to be done one at a time I guess.

Note: There was a better way, mentioned below. I could have written a new method for dictionary parsing, and called the new one and the old one, moving the dictionary-based items one at a time. Very incremental, basically Strangler pattern. Pity that I didn’t think of it, we wold all have been impressed. This goes well enough anyway.

This much gets the add bot test running:

    def unpack_and_execute(self, entity, **parameters):
        if entity:
            self.ids_used.add(entity)
            parameters['entity_object'] = self.entity_from_id(entity)
        else:
            parameters['entity_object'] = None
        self.execute_action(parameters)

    def execute_action(self, action_dictionary):
        match action_dictionary:
            case {'verb': 'add_bot', 'x': x, 'y': y, 'direction': direction}:
                self.add_bot_action(x, y, direction)
            ...

What I like about this is that the detailed parsing happens earlier, it is less procedural, and the called methods get just what they want.

This moves us from nine failures to six:

    case {'verb': 'step', 'entity_object': entity_object}:
        self.step(entity_object)

Moving right along:

Down to five with this:

    case {'verb': 'take', 'entity_object': entity_object}:
        self.take_forward(entity_object)

I note some duplication forming. If we were to parse entity_object out in the function parameters, we could remove those values from the dictionary. We’ll see about that later.

A couple more taps with the hammer and we are green:

    def unpack_and_execute(self, entity, **parameters):
        if entity:
            self.ids_used.add(entity)
            parameters['entity_object'] = self.entity_from_id(entity)
        else:
            parameters['entity_object'] = None
        self.execute_action(parameters)

    def execute_action(self, action_dictionary):
        match action_dictionary:
            case {'verb': 'add_bot', 'x': x, 'y': y, 'direction': direction}:
                self.add_bot_action(x, y, direction)
            case {'verb': 'drop', 'entity_object': entity_object, 'holding': holding}:
                self.drop_forward_action(entity_object, holding)
            case {'verb': 'step', 'entity_object': entity_object}:
                self.step(entity_object)
            case {'verb': 'take', 'entity_object': entity_object}:
                self.take_forward(entity_object)
            case {'verb': 'turn','entity_object': entity_object, 'direction': direction}:
                self.turn(entity_object,direction)
            case {'entity_object': entity_object,
                  'verb': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction}:
                self.turn(entity_object, direction)
            case _:
                raise Exception(f'Unknown action {action_dictionary}')

Clearly we can move the entity_object selection up top, like this:

    def unpack_and_execute(self, entity, **parameters):
        if entity:
            self.ids_used.add(entity)
            parameters['entity_object'] = self.entity_from_id(entity)
        else:
            parameters['entity_object'] = None
        self.execute_action(**parameters)

    def execute_action(self, entity_object, **action_dictionary):
        action_dictionary['entity_object'] = entity_object
        match action_dictionary:
            case {'verb': 'add_bot', 'x': x, 'y': y, 'direction': direction}:
                self.add_bot_action(x, y, direction)
            case {'verb': 'drop', 'entity_object': entity_object, 'holding': holding}:
                self.drop_forward_action(entity_object, holding)
            case {'verb': 'step', 'entity_object': entity_object}:
                self.step(entity_object)
            case {'verb': 'take', 'entity_object': entity_object}:
                self.take_forward(entity_object)
            case {'verb': 'turn','entity_object': entity_object, 'direction': direction}:
                self.turn(entity_object,direction)
            case {'entity_object': entity_object,
                  'verb': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction}:
                self.turn(entity_object, direction)
            case _:
                raise Exception(f'Unknown action {action_dictionary}')

Above, I switch to passing in the unpacked dictionary in unpack_and_execute, and parse out the entity_object in the execute_action parameter list. Then, to keep the tests running, I stuff it back into the dictionary. Now I think we’ll commit a few times, since we’re green and I think we like this well enough to keep it. Commit: using action dictionary in command parsing match/case.

Then:

    def execute_action(self, entity_object, **action_dictionary):
        match action_dictionary:
            case {'verb': 'add_bot',
                  'x': x, 'y': y, 'direction': direction}:
                self.add_bot_action(x, y, direction)
            case {'verb': 'drop',
                  'holding': holding}:
                self.drop_forward_action(entity_object, holding)
            case {'verb': 'step'}:
                self.step(entity_object)
            case {'verb': 'take'}:
                self.take_forward(entity_object)
            case {'verb': 'turn',
                  'direction': direction}:
                self.turn(entity_object,direction)
            case {'verb': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction}:
                self.turn(entity_object, direction)
            case _:
                raise Exception(f'Unknown action {action_dictionary}')

Green. Commit: parsing out and using entity_object.

One more tiny change:

    def unpack_and_execute(self, entity, **parameters):
        if entity:
            self.ids_used.add(entity)
            parameters['entity_object'] = self.entity_from_id(entity)
        self.execute_action(**parameters)

    def execute_action(self, entity_object=None, **action_dictionary):
        match action_dictionary:
            case {'verb': 'add_bot',
                  'x': x, 'y': y, 'direction': direction}:
                self.add_bot_action(x, y, direction)
            case {'verb': 'drop',
                  'holding': holding}:
                self.drop_forward_action(entity_object, holding)
            case {'verb': 'step'}:
                self.step(entity_object)
            case {'verb': 'take'}:
                self.take_forward(entity_object)
            case {'verb': 'turn',
                  'direction': direction}:
                self.turn(entity_object,direction)
            case {'verb': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction}:
                self.turn(entity_object, direction)
            case _:
                raise Exception(f'Unknown action {action_dictionary}')

We provide an entity_object if we get a non-zero id, otherwise we do not. And execute_action defaults it to None if it isn’t provided.

Is that really better? I suspect we’ll get a weird crash either way. Let’s write a test before I commit this.

    def test_zero_id(self):
        WorldEntity.next_id = 100
        world = World(10, 10)
        command = {'entity': 0,
                   'verb': 'step'}
        rq = [ command ]
        result = world.execute(rq)
        assert len(result) == 1

Here we just try to move a bot whose id is zero. We hurl:

    def step(self, bot):
>       self.map.attempt_move(bot.id, bot.forward_location())  # changes world version
E       AttributeError: 'NoneType' object has no attribute 'id'

We’re not handling errors during command execution yet, so for now let’s show what we get:

    def test_zero_id(self):
        WorldEntity.next_id = 100
        world = World(10, 10)
        command = {'entity': 0,
                   'verb': 'step'}
        rq = [ command ]
        with pytest.raises(AttributeError) as error:
            result = world.execute(rq)
        assert str(error.value) == "'NoneType' object has no attribute 'id'"

I think we’re good. Here’s our method:

    def execute_action(self, entity_object=None, **action_dictionary):
        match action_dictionary:
            case {'verb': 'add_bot',
                  'x': x, 'y': y, 'direction': direction}:
                self.add_bot_action(x, y, direction)
            case {'verb': 'drop',
                  'holding': holding}:
                self.drop_forward_action(entity_object, holding)
            case {'verb': 'step'}:
                self.step(entity_object)
            case {'verb': 'take'}:
                self.take_forward(entity_object)
            case {'verb': 'turn',
                  'direction': direction}:
                self.turn(entity_object,direction)
            case {'verb': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction}:
                self.turn(entity_object, direction)
            case _:
                raise Exception(f'Unknown action {action_dictionary}')

Commit: move default of zero id into execute_action. But I do see one more thing we can improve:

    def execute_action(self, entity_object=None, **action_dictionary):
        match action_dictionary:
            case {'verb': 'add_bot',
                  'x': x, 'y': y, 'direction': direction}:
                self.add_bot_action(x, y, direction)
            case {'verb': 'drop',
                  'holding': holding}:
                self.drop_forward_action(entity_object, holding)
            case {'verb': 'step'}:
                self.step(entity_object)
            case {'verb': 'take'}:
                self.take_forward(entity_object)
            case {'verb': 'turn',
                  'direction': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction}:
                self.turn(entity_object,direction)
            case {'verb': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction}:
                self.turn(entity_object, direction)
            case _:
                raise Exception(f'Unknown action {action_dictionary}')

I provided the legal cases for ‘direction’ in the ‘turn’ command.

Time’s up, and with all this code showing, the article is lengthy. I hope it’s not difficult, just long.

Summary

We experimented a bit with using match/case with a dictionary, and then converted our action handling to use dictionary parsing in the execute_action logic. In a couple of cases, we took interim steps, but it finally came down to converting each of the cases, one at a time, watching more and more tests run.

Was there a more incremental way? I can think of one:

We could have written a new version of execute_action, execute_action_dict, and called that as well as calling the existing eecute_action. Then we could add dictionary parsing to the new method, one at a time. In essence it would be a tiny case of the strangler pattern.

I really wish I had thought of that: it would have impressed us all. But I didn’t. Life is still good.

The new execute_action method is far more explicit about the syntax of the various commands, and there are some interesting possibilities. For example:

    def test_wrong_turn(self):
        world = World(10, 10)
        bot_id = world.add_bot(5, 5)
        bot = world.entity_from_id(bot_id)
        rq = [ {'entity': bot_id,
                'verb': 'turn',
                'direction': 'AROUND'}]
        with pytest.raises(AttributeError) as error:
            world.execute(rq)
        assert str(error.value) == 'unknown direction AROUND'

We can pass this test with a new case:

        case {'verb': 'turn',
              'direction': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction}:
            self.turn(entity_object,direction)
        case {'verb': 'turn', 'direction': bad_direction}:
            raise AttributeError(f'unknown direction {bad_direction}')

So that’s nice. Commit: improve a diagnostic for bad turn direction.

I am pleased with how this is shaping up. Our pattern matching is far more explicit, and we can detect special cases and error conditions more declaratively. Nothing we couldn’t have done with procedural code, but I think this matching is better and “more Pythonic”.

I feel that I’m failing better over the past couple of days. Very pleasing! See you next time!