Review

Setting out to review the code, I discover my old InputBuilder experiment. I go one way then, the other, then back. An actual revert of a commit. No new code, but some useful thinking.

I’m moving directly here from the preceding article, with just a quick walk around the house as a break.

I have a few things that we should look at:

1. Can we quickly move from our one action per request to multiple actions in one request?
2. Is there general code clean-up that we need?
3. Is it time to do a builder?
4. Is there code in World that needs revision?
5. Are there tests against World that need revision?

You may recall that I’ve done some experiments with a request builder, none quite like the request we’re building but they certainly seem promising. I ran across this code in World:

    def update_client_for_test(self, client_bot):
        result_dict = self.fetch(client_bot.id)
        client_bot._knowledge.update(result_dict)

    def process(self, world_input):
        from tests.test_world_batch import WorldOutput
        output = WorldOutput()
        for request in world_input:
            requestor_id = request.identifier
            for action in request:
                self.command(action.action, requestor_id, action.parameter)
            output.append(self.fetch(requestor_id))
        return output

    def command(self, action, bot_id, parameter=None):
        world_bot = self.entity_from_id(bot_id)
        if action == 'step':
            self.step(world_bot)
        elif action == 'take':
            self.take_forward(world_bot)
        elif action == 'drop':
            block = self.entity_from_id(parameter)
            self.drop_forward(world_bot, block)
        elif action == 'turn':
            self.set_direction(world_bot, parameter)
        else:
            raise Exception(f'Unknown command {action}')

The first method may lead us to some tests that need improvement. The second and third clearly need to be removed or replaced with the new execute and execute_command. The process method is the only user of command. Let’s see who’s using process:

Ah, this is interesting, and a bit challenging. There are five tests that are creating instances of my experimental classes for building requests, and running them through the old command processing.

Just to see what will happen, I will run them through the new execute method, if I can wire it up quickly.

    def process(self, world_input):
        from tests.test_world_batch import WorldOutput
        output = WorldOutput()
        for request in world_input:
            requestor_id = request.identifier
            self.execute(request)
            output.append(self.fetch(requestor_id))
        return output

Four tests fail. I expected plenty of failures: the structures surely don’t quite line up.

    def execute(self, request):
>       id = request["entity"]
E       TypeError: 'EntityRequest' object is not subscriptable

What is that class like?

class EntityRequest:
    def __init__(self, entity_identifier, world_input=None):
        self._world_input = world_input
        self._entity_identifier = entity_identifier
        self._actions = []

    def add_action(self, action: EntityAction):
        self._actions.append(action)

    @property
    def identifier(self):
        return self._entity_identifier

    def __iter__(self):
        return iter(self._actions)

We could make it subscriptable.

    def __getitem__(self, item):
        if item == 'entity':
            return self._entity_identifier
        raise Exception(f'cannot get {item}')

Let’s see what we find.

E       Exception: cannot get actions

    def __getitem__(self, item):
        if item == 'entity':
            return self._entity_identifier
        elif item == 'actions':
            return self._actions
        raise Exception(f'cannot get {item}')

One of the tests passes, three still fail.

    def execute_action(self, entity, action):
>       verb = action["verb"]
E       TypeError: tuple indices must be integers or slices, not str

OK this was interesting but not as interesting as it might be. Let’s roll back, just to clear my mind.

This time, let’s see if we can make our builder build a request the way we like them.

If we can, then we can change process as we did above and run it all through the existing execute logic. Here’s the most robust test, the one we’d actually like to have run:

    def test_new_syntax(self):
        world = World(10, 10)
        block_id = world.add_block(6, 5)
        batch_in = InputBuilder(world) \
            .request(world.add_bot(5, 5, direction=Direction.EAST)) \
                .action('take', None) \
                .action('turn', 'SOUTH') \
                .action('step', None) \
                .action('step', None) \
                .action('drop', block_id) \
            .request(world.add_bot(7, 7)) \
                .action('step') \
            .result()
        batch_out = world.process(batch_in)
        result = batch_out.results[0]
        assert result['location'] == Location(5, 7)
        result = batch_out.results[1]
        assert result['location'] == Location(8, 7)
        item = world.map.at_xy(5, 8)
        assert item.id == block_id

That code relies on InputBuilder, the experimental class that builds the highly structured object referenced in process.

Huh!

I’m about to make a mistake. The InputBuilder builds a rather nice structured object:

WorldInput has a collection of
    EntityRequest, which has 
        identifier: an entity id
        actions, a collection of
            EntityAction, which has
                action: a verb
                parameter: whatever

I was about to change things so that the builder would create the dictionary structure that execute currently uses. It would be far better to adapt the World to use these new objects. And … well … the fact that these tests run suggest that it already does.

We’ll compare the execution parts:

    def execute_action(self, entity, action):
        verb = action["verb"]
        match verb:
            case 'step':
                self.step(entity)
            case 'take':
                self.take_forward(entity)
            case 'drop':
                holding_id = action["param1"]
                holding = self.entity_from_id(holding_id)
                self.drop_forward(entity, holding)
            case 'turn':
                direction = action["param1"]
                self.set_direction(entity, direction)
            case _:
                raise Exception(f'Unknown action {action}')

    def command(self, action, bot_id, parameter=None):
        world_bot = self.entity_from_id(bot_id)
        if action == 'step':
            self.step(world_bot)
        elif action == 'take':
            self.take_forward(world_bot)
        elif action == 'drop':
            block = self.entity_from_id(parameter)
            self.drop_forward(world_bot, block)
        elif action == 'turn':
            self.set_direction(world_bot, parameter)
        else:
            raise Exception(f'Unknown command {action}')

Very similar. The process scheme relies on the fact that it can fetch the parameter unconditionally, and it comes back silently as None if there is none.

As an experiment, let’s try changing one of DirectConnection’s methods to create a WorldInput type request. Before we do that, I think we should promote these builder and built objects out of the tests into at least one production file.

This is going to be a bit tricky. I’ve already tried and rolled back once. I think I have the files the way I want but the tests can’t build. I’ll need to sort out imports. Importing namedtuple helps. Now just a few are broken.

Found one more import to fix and we are back to green with the objects moved into prod. Commit: moving world request spike objects to prod, preparing to use them.

OK, I need to clear my head. Let’s think about what we’re going to do.

We have this nice InputBuilder class that builds the structured request objects I described above. The plan was, a few moments ago, to use them. There is, I think, an issue that I wish I had thought of before I did this most recent commit.

The connections will be sending JSON back and forth. JSON is really good at sending things like dictionaries back and forth. It is not so good at sending and receiving structured objects.

This is no good. I am not convinced that these structured objects are the way to go, given client server. If we didn’t have the JSON in the middle, I would decide differently.

Huh-squared

I was wrong above when I thought I was mistaken. I think the dictionary is actually better for our situation.

Bah. Revert that commit.

So. What about that process code and the InputBuilder and all that jazz? Well, they are a good spike from which to learn, but I think we really do want an InputBuilder that just builds a structured dictionary. Much as I prefer having objects of my own rather than system collections … I think that in this case, with the JSON bridge in between and because my client is really an example of a client, using the dictionary makes more sense.

So we have thought about this, tried a few things, and decided …

I think what I’ll do is build up the dictionary, and a list of dictionaries, to be sent across. Whether I’ll create a convenient builder first, I’m not sure. It would be nice to have, and I do like the look of the code that uses the existing builder:

    batch_in = InputBuilder(world) \
        .request(world.add_bot(5, 5)) \
            .action('take') \
            .action('turn','SOUTH') \
            .action('step') \
            .action('step') \
            .action('drop', block_id) \
        .request(world.add_bot(7, 7)) \
            .action('step') \
            .action('step') \
        .result()

I’ll decide that later. For now, I need a snack. See you next time!