Recovery
After yesterday’s
debacleminor hiccup, let’s continue to do a bit of error handling. We do some good stuff and perhaps have a better idea.
OK, after shipping broken code yesterday morning, I forgot to ship yesterday afternoon’s article. It’s up now, entitled Yikes! for your interest and delectation. Not a perfect day. We’ll see if we can be a bit more adept today.
- Eptitude
- We are all variously ept1 from day to day, sometimes really rocking it and other times struggling to find the arm holes in our jacket. I think that the most valuable thing we can do is to pay attention to how we’re feeling and how we’re doing, and to adjust the difficulty of what we undertake to our best guess at our current eptness. Yesterday, I felt just fine and still made two significant oversights.
-
PyCharm will help me avoid pushing code that doesn’t run the tests. But it will warn me about broken tests, not just on failures but also on ignores. I wonder if it can be adjusted not to do that. Yes, the right thing will be to get rid of the ignores. I’ll do that. But still, pushing with ignores is OK in my practice, so if PyCharm can deal with that, it would be good.
-
It appears that PyCharm cannot be adjusted to push with ignored tests but not with failures. I’ll turn on the feature and see what’s up. Then I’ll fix things up. I’ll try to remember to mention what I decide to do.
Today I thought I might see if this method can be improved:
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)
case {'verb': 'step'}:
self.step_action(entity_object)
case {'verb': 'take'}:
self.take_forward_action(entity_object)
case {'verb': 'turn',
'direction': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction}:
self.turn_action(entity_object, direction)
case {'verb': 'turn', 'direction': bad_direction}:
raise AttributeError(f'unknown direction {bad_direction}')
case {'verb': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction}:
self.turn_action(entity_object, direction)
case _:
self._add_message(f'Unknown action {action_dictionary}')
My concern with this is small2, but our mission here includes noticing small things and trying to make them better. It’s not that in a real project we could take the time to be this nit-pickish3 all the time, but if we sharpen our perceptions and become good at small changes, we can spot and deal with more small things and thus leave our code space in better order. Every little bit helps.
Anyway my concern is with this bit:
case {'entity_object': None }:
verb = action_dictionary.get('verb', 'missing verb')
self._add_message(f'verb {verb} requires entity parameter {action_dictionary}')
It’s down in the middle of things, and its job is to produce an error message for actions that need but do not have an entity_object. I think it’s a fairly elegant way to do it, but it’s far from clear what’s going on just from reading it. I even felt the need to put in a comment! The shame!
So I’m thinking that at about this point in the code, or perhaps the method above, we would split into two methods, one that deals with cases that don’t need an entity object, and the cases that do. We might or might not still use the check as shown here.
Hm. I think I see another way. What if we pay the small price of including an explicit check for entity object in each of the cases? If we did that, any case that needs it would drop through if it was not provided. We could let the bottom case _ catch them, or we could catch them individually. Let’s try that just to see how it looks.
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 {'entity_object': entity_object,
'verb': 'drop',
'holding': holding}:
self.drop_forward_action(entity_object, holding)
case {'entity_object': entity_object,
'verb': 'step'}:
self.step_action(entity_object)
case {'entity_object': entity_object,
'verb': 'take'}:
self.take_forward_action(entity_object)
case {'entity_object': entity_object,
'verb': 'turn',
'direction': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction}:
self.turn_action(entity_object, direction)
case {'verb': 'turn',
'direction': bad_direction}:
raise AttributeError(f'unknown direction {bad_direction}')
case {'entity_object': entity_object,
'verb': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction}:
self.turn_action(entity_object, direction)
case _:
self._add_message(f'Unknown action {action_dictionary}')
I removed the special check for ‘entity_object’, and added it to all the cases that need it. And I changed the entry point not to break out entity_object, adjusting the caller as well.
We have two broken tests. This one is no surprise:
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']
Maybe I still like that check. After a bit of reformatting, I find this solution:
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_action(entity_object)
case {'verb': 'take',
'entity_object': entity_object}:
self.take_forward_action(entity_object)
case {'verb': 'turn',
'entity_object': entity_object,
'direction': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction}:
self.turn_action(entity_object, direction)
case {'verb': 'turn',
'direction': bad_direction}:
raise AttributeError(f'unknown direction {bad_direction}')
case {'verb': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction,
'entity_object': entity_object}:
self.turn_action(entity_object, direction)
case { 'verb': verb} if verb not in ['add_bot',]:
self._add_message(f'verb {verb} requires entity parameter {action_dictionary}')
case _:
self._add_message(f'Unknown action {action_dictionary}')
I moved all the verbs to be first in the case, which I think aids in reading. And now there are two error-detecting cases (so far), and they are down at the bottom. The first one relies on the other cases selecting ‘entity_object’, and does assume that no one has accidentally forgotten it above. I think that’s quite unlikely, but it could happen.
We are green. Let’s commit this: refactoring execute_action. PyCharm reminds me about ignored tests. Let’s find them. They are in the TestDecorators file that I made when I was learning about decorators. I’ll keep the file but comment out those tests. Commit: comment out xfail tests to allow push without complaint. PyCharm runs the tests before pushing and remains happy. I am happy too.
Now let’s address this exception as part of our message improvements:
case {'verb': 'turn',
'entity_object': entity_object,
'direction': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction}:
self.turn_action(entity_object, direction)
case {'verb': 'turn',
'direction': bad_direction}:
raise AttributeError(f'unknown direction {bad_direction}')
If the direction is legit, the first case fires. If it isn’t, the second fires. We need a test:
def test_invalid_turn_message(self):
world = World(10, 10)
bot_id = world.add_bot(5, 5)
requests = [{'verb': 'turn', 'direction': 'LEFT'}]
messages = world.execute_requests(requests)['messages']
expected = 'valid directions are NORTH, EAST, SOUTH, and WEST'
assert expected in messages[0]['message']
This fails currently:
case {'verb': 'turn',
'direction': bad_direction}:
> raise AttributeError(f'unknown direction {bad_direction}')
E AttributeError: unknown direction LEFT
Hm, let’s include the incorrect value in the message. Change the test:
def test_invalid_turn_message(self):
world = World(10, 10)
bot_id = world.add_bot(5, 5)
requests = [{'verb': 'turn', 'direction': 'LEFT'}]
messages = world.execute_requests(requests)['messages']
expected = 'unknown direction LEFT, should be NORTH, EAST, SOUTH, or WEST'
assert expected in messages[0]['message']
And:
case {'verb': 'turn',
'direction': bad_direction}:
self._add_message(f'unknown direction {bad_direction}, should be NORTH, EAST, SOUTH, or WEST')
A different test fails. I was a redundant test that expected the exception. Removed it. Green. Commit: added message for wrong turn.
I am concerned about our test with the mistaken verb. Let’s check it.
def test_no_verb(self):
world = World(10, 10)
requests = [{'entity_object': "fake", 'vorb': 'add_bot'}]
messages = world.execute_requests(requests)['messages']
assert 'Unknown action' in messages[0]['message']
Why is this passing? Why didn’t the first error-checker catch it:
case { 'verb': verb} if verb not in ['add_bot',]:
self._add_message(f'verb {verb} requires entity parameter {action_dictionary}')
case _:
self._add_message(f'Unknown action {action_dictionary}')
Oh that wasn’t a bad verb it was a bad word name. That’s the best we can do for that. I do think that we’ll get a weird message for a non-verb though:
def test_bad_verb(self):
world = World(10, 10)
requests = [{'entity_object': "fake", 'verb': 'whatever'}]
messages = world.execute_requests(requests)['messages']
assert 'unknown verb: whatever' in messages[0]['message']
This fails, no surprise. I expect the entity parameter message, which we do not want.
> assert 'unknown verb: whatever' in messages[0]['message']
E assert 'unknown verb: whatever' in "verb whatever requires entity parameter {'entity_object': 'fake', 'verb': 'whatever'}"
How can we manage this? I don’t see a better way than to have a list of all verbs. Let’s try that, since it is a way.
case { 'verb': verb } if verb not in self.all_verbs:
self._add_message(f'unknown verb: {verb}')
case { 'verb': verb} if verb not in ['add_bot',]:
self._add_message(f'verb {verb} requires entity parameter {action_dictionary}')
case _:
self._add_message(f'Unknown action {action_dictionary}')
@property
def all_verbs(self):
return [
'add_bot',
'drop',
'step,'
'take',
'turn',
'NORTH',
'EAST',
'SOUTH',
'WEST',
]
Green. Commit: improve error message. Urrk. Commit check finds two tests failing. I somehow missed that. Good job PyCharm.
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 gets the unknown verb message:
Expected :'unknown verb: step'
Actual :'requires entity parameter'
The message is inverted, use of ‘in’ seems to confuse pytest.
def test_not_in(self):
world = World(10, 10)
assert 'step' in world.all_verbs
This test fails. Ha!
Expected :['add_bot', 'drop', 'step,take', 'turn', 'NORTH', 'EAST', 'SOUTH', 'WEST']
Actual :'step'
I think we have some misplaced quote marks here!
@property
def all_verbs(self):
return [
'add_bot',
'drop',
'step',
'take',
'turn',
'NORTH',
'EAST',
'SOUTH',
'WEST',
]
Green. Commit as before.
Let’s have one more look at the method and then reflect and sum up.
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_action(entity_object)
case {'verb': 'take',
'entity_object': entity_object}:
self.take_forward_action(entity_object)
case {'verb': 'turn',
'entity_object': entity_object,
'direction': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction}:
self.turn_action(entity_object, direction)
case {'verb': 'turn',
'direction': bad_direction}:
self._add_message(f'unknown direction {bad_direction}, should be NORTH, EAST, SOUTH, or WEST')
case {'verb': 'NORTH' | 'EAST' | 'SOUTH' | 'WEST' as direction,
'entity_object': entity_object}:
self.turn_action(entity_object, direction)
case { 'verb': verb } if verb not in self.all_verbs:
self._add_message(f'unknown verb: {verb}')
case { 'verb': verb} if verb not in ['add_bot',]:
self._add_message(f'verb {verb} requires entity parameter {action_dictionary}')
case _:
self._add_message(f'Unknown action {action_dictionary}')
Reflective Summary
I am concerned, of course with the Wall Of Code Pattern we see there. One way or another, we’re going to have a big message dispatch on verb but does it have to be that messy?. I could imagine some table of lambdas or the like, but that’s likely to get very messy because of the slight but real differences in the calling sequences of the various actions.
- Note
- I was going to let the Wall Of Code issue go, since I am somewhat proud of the use of the dictionary in the case statements. But thinking about what could be better gives me an idea that’s worth trying. Read on.
I am not entirely pleased with the need for the all_verbs property, but I’m willing to pay the price for the improved message when you give us a verb that we don’t recognize.
The big block of code of the match/case doesn’t please me, and while I prefer it with the extra white space, the very need for it is an indication that we have a nasty chunk of code here.
Another conceivable approach might be some code something like this:
def execute_action(self, action_dictionary):
verb = action_dictionary.get('verb')
match verb:
case 'add_bot':
self.add_bot(action_dictionary)
case 'drop':
self.drop(action_dictionary)
case 'step':
self.step(action_dictionary)
...
case _: self._add_message('Unknown verb {verb}')
With some griping from PyCharm, we could even put it this way:
def execute_action(self, action_dictionary):
verb = action_dictionary.get('verb')
match verb:
case 'add_bot': self.add_bot(action_dictionary)
case 'drop': self.drop(action_dictionary)
case 'step': self.step(action_dictionary)
...
case _: self._add_message('Unknown verb {verb}')
That would be pretty compact either way. If we expanded the dictionary into each called method, like this:
def execute_action(self, action_dictionary):
verb = action_dictionary.get('verb')
match verb:
case 'add_bot': self.add_bot(**action_dictionary)
case 'drop': self.drop(**action_dictionary)
case 'step': self.step(**action_dictionary)
...
case _: self._add_message('Unknown verb {verb}')
We might be able to put fairly simple parsing into the individual methods, using the calling sequence unpacking trick.
You know what? That might be worth trying. We’re here, not to save time and get done in a hurry. We’re here to find good ways of doing things so that when we are doing Real Programming, we’ll have better ideas and experience, all ready to go.
We’ll try that idea next time, I think. It might be better. We might be glad we thought about this. See you then!