Yikes!
I was just going to make a small change, and tests are failing. How did this happen?
I was certain these were all running when I committed. Oh well, let’s see what’s up.
def test_no_entity(self):
world = World(10, 10)
requests = [ {'verb': 'step'}]
messages = world.execute_requests(requests)['messages']
assert 'requires entity parameter' in messages[0]['message']
This is getting an exception:
def step_action(self, bot):
> self.map.attempt_move(bot.id, bot.forward_location()) # changes world version
E AttributeError: 'NoneType' object has no attribute 'id'
We have a None for the parameter bot. I would have sworn that was the thing we just made work. I wonder if something reverted.
def execute_action(self, action_dictionary):
entity_object = action_dictionary.get('entity_object', None)
match action_dictionary:
case {'verb': 'add_bot',
'x': x, 'y': y, 'direction': direction}:
self.add_bot_action(x, y, direction)
# -----------------------------------------------
# operations below here all require entity_object
case {'entity_object': None }:
verb = action_dictionary.get('verb', 'missing verb')
self._add_message(f'verb {verb} requires entity parameter {action_dictionary}')
case {'verb': 'drop',
'holding': holding}:
self.drop_forward_action(entity_object, holding)
case {'verb': 'step'}:
self.step_action(entity_object)
that sure looks to me as if we’ll have an entity_object that is None and that the case below the comment should find it. Oh … not if it is missing. It needs to be in the dictionary. This fixes both tests:
def execute_action(self, action_dictionary):
entity_object = action_dictionary.get('entity_object', None)
action_dictionary['entity_object'] = entity_object
Wow. The last thing I did in the prior article was to remove that line that set the value back into the dictionary. Did the tests fail and I just didn’t notice? I thought we ran tests before committing.
Anyway, that’s the fix, or at least one possible fix. Commit: fix defect breaking tests and game. Mea culpa.
Let’s extract an Explaining Method:
def execute_action(self, action_dictionary):
entity_object = self.ensure_dictionary_has_entity_object(action_dictionary)
match action_dictionary:
case {'verb': 'add_bot',
'x': x, 'y': y, 'direction': direction}:
self.add_bot_action(x, y, direction)
# -----------------------------------------------
# operations below here all require entity_object
case {'entity_object': None }:
verb = action_dictionary.get('verb', 'missing verb')
self._add_message(f'verb {verb} requires entity parameter {action_dictionary}')
case {'verb': 'drop',
'holding': holding}:
...
def ensure_dictionary_has_entity_object(self, action_dictionary):
entity_object = action_dictionary.get('entity_object', None)
action_dictionary['entity_object'] = entity_object
return entity_object
Commit: Explaining Method.
Is there a more compact way to write that little method? I don’t see one. We’ll let that ride for now. Or, should we go back to the previous scheme?
def execute_action(self, entity_object=None, **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)
# -----------------------------------------------
# operations below here all require entity_object
case {'entity_object': None }:
verb = action_dictionary.get('verb', 'missing verb')
self._add_message(f'verb {verb} requires entity parameter {action_dictionary}')
case {'verb': 'drop',
'holding': holding}:
self.drop_forward_action(entity_object, holding)
Yes, I think I prefer that after all. Never should have changed it. Green. Commit: refactoring.
I don’t like the fact that that line does not explain itself, but if it gets removed a test breaks. It’s kind of a hack, allowing the check for entity object parameter to work, since we can only check the dictionary at this point. Maybe if we split the match/case, as we mentioned last time, it would be better. Good enough for now.
Now I was going to require an action to be a dictionary. Let’s add a test.
def test_action_must_be_dict(self):
world = World(10, 10)
requests = [ {'verb', 'step'}]
messages = world.execute_requests(requests)['messages']
assert 'must be dictionary' in messages[0]['message']
This fails, of course:
> action_with_parameters = self.assign_parameters(**action)
E TypeError: server.world.World.assign_parameters() argument after ** must be a mapping, not set
Let’s just check:
def execute_actions(self, actions_list):
for action in actions_list:
if isinstance(action, dict):
action_with_parameters = self.assign_parameters(**action)
self.execute_action(**action_with_parameters)
else:
self._add_message(f'action must be dictionary {action}')
That passes nicely. We could use try/except, but that’s too generic unless we want to sort out a number of different exceptions. We’ll let that wait until we do a top-level-all-else-fails try/except.
Green, commit: message returned if action is not a dictionary.
Summary
OK, that’s all I had intended to do, the dictionary check. I’m glad that I set out to do it and embarrassed that there were tests failing. I must have forgotten to pin the test tab or something.
I’ve tried setting the PyCharm commit configuration to require the tests to run, but that objects to ignored ones. I’ll have to clear those up before I can readily use that flag.
Live and learn, but if I keep breaking the build like this, I’m going to get a very poor employee review. I must do better, somehow.
See you next time!