A Scheme
Hello, loves!
Not even that devious: this seems like a decent idea for the simplified Content.
The AppearingContent object gave me some trouble using the new CombinedContent experimental object, though I was able to make it work. We’ll look at the offending code in a moment but as I see it the issues include:
- Setting up the objects is a bit gnarly. I suspect that the idea of using
nonlocalto get access to objects that we need to recall is contributing. We may go back to providing aninfopackage and using that instead. - Our subscription callbacks need access to PubSub and in general there is no guarantee that they’ll be in a place where that access is possible.
- Relatedly, it would be nice if we could get rid of Dungeon’s methods that forward to PubSub.
- A simple, easy-to-make mistake can create a recursion in publish-subscribe, if an object publishes something to which it subscribes. We need to make that less likely. Ideally it would be impossible but as yet, I see no good way to eliminate the possibility entirely.
Let’s review the code that makes AppearingObject work in CombinedContent class, and see what we can see:
def test_appearing(self):
name = 'a thing'
visible = False
def interaction(self, interactor):
nonlocal visible
if visible:
interactor.receive_content(self)
interactor.announce(f'You have received {self.name}!')
return True
def appear(content, state):
nonlocal visible
content.dungeon.announce(f'{name} has appeared!')
content.dungeon.publish('state_number', 'x', content=self, state=state)
visible = True
sub = Subscription(event='state_number',
caller_id=name,
callback=appear,
one_shot=True)
item = CombinedContent(name=name,
resources=['a', 'b'],
scale=0.5,
interaction=interaction,
subs=[sub]
)
item.run(self)
# item.interact_with_player(self)
assert self.message is None
self.pub_sub.publish('state_number', name, content=item, state=1)
assert self.message == 'a thing has appeared!'
In the code above, we need an extra “instance variable”, visible, in addition to the usual resources, scale, and so on. We create it in the setup, which would typically be a class method or factory method, and then use nonlocal to give our code access to it. That works, but it’s weird, and I really have little idea what Python has to do to retain all that context just to support that boolean variable.
The code is confusing in a way that I just noticed: name is local to the creation, but visible is retained and used inside the callbacks.
All this convinces me that the idea of creating an info package will be better.
However, it may not be obvious here, but the interaction function runs as a method of CombinedCOntent, while the callback function appear does not. This means that even if info were used, the appear callback would have no access to it.
This convinces me that the callbacks in our subscriptions list should be bound to the content item as methods, so as to have a self with access to whatever variables they need to access.
Finally, the content.dungeon.announce and .publish are a crock, in that there is special code in CombinedContent.run to cache the Dungeon instance in a member variable. This gives it a pointer back to its ultimate container, and while that can be convenient, it is also a very questionable design property.
This convinces me that all PubSub callback calling sequences should begin with the PubSub instance, so that further subscriptions and announcements can be made. Additionally, rather than have a method on Dungeon announce, code that wants an announcement should publish a message requesting it, and some object somewhere should subscribe to that and do the work.
Q: This seems like a lot?
A: Yeah, there’s a bit to do there, probably more than it looks like.
Q: Isn’t that bad?
A: No, it’s good. In only about 90 minutes of coding and article writing, we’ve learned all those things.
Q: Um … OK … now what?
A: Now we get the pleasure of doing better than we did yesterday. Let’s get to it!
I can identify two things that are critical to the new scheme we’re formulating here. One is that we want all the callbacks to expect a PubSub instance as their first (non-self) parameter. The other is that we want the subscriptions we provide in the CombinedContent subscriptions parameter to be bound to the object as methods, allowing (and requiring) those callbacks to begin with a self parameter.
The first change is a bit global in nature, as there are, just a moment while I count: upwards of twenty calls to subscribe, counting tests, and each of those callbacks will have to be fixed up.
Now all here know that I believe that any “large” refactoring can be done in small steps, over a long period of time, while allowing other changes to proceed as usual. How could that possibly be done in this case?
We would enhance the PubSub object to have two kinds of events in the lists where it keeps subscriptions, the ones that are ready for the new parameter and the ones that are not. We would provide a new subscribe method that tells PubSub the caller is ready for the PubSub parameter, and make its entry one of the new kind. Then, over any period of time, we could update a given callback, change the subscribe call to the new one, rinse, repeat. When the last one is done, we’re dong, and we can unwind the changes to PubSub and, ideally, rename the new method throughout.
We’re not going to do that, but we could. What we’re going to do is just make the change all at once.
Here is the one method in PubSub that needs to be changed:
class PubSub(PubSubProtocol):
def execute_callbacks(self, key, kwargs):
try:
subscriptions = list(self.subscriptions[key])
for subscription in subscriptions:
subscription.callback(**kwargs)
if subscription.one_shot:
self.subscriptions[key].remove(subscription)
except KeyError:
pass
And here is the changed version, which will break, just a moment while I count, seven tests.
def execute_callbacks(self, key, kwargs):
try:
subscriptions = list(self.subscriptions[key])
for subscription in subscriptions:
subscription.callback(pub_sub=self, **kwargs)
if subscription.one_shot:
self.subscriptions[key].remove(subscription)
except KeyError:
pass
I am confident that it breaks more code than those seven but let’s get those running first. Here’s the kind of change we need, adding the single parameter pub_sub:
def test_subscriptions(self):
count = 0
def increment(*, pub_sub, content, state):
nonlocal count
print('counting')
count += 1
sub = Subscription(event='do_it', caller_id='', callback=increment)
...
Six failing. Some of them are in our new content tests. We’ll leave those until the end, as we want to give them more attention.
Since all we needed to do was add a pub_sub parameter, we’ll skip the details. Down to one test failing, in the new content tests. I did have to change a couple of the lines in the ‘src’ tree, but of course we have to find and change all of those anyway.
The new test’s change didn’t require too much attention, but we plan to redo the whole thing with our new improvements. Here’s all I changed:
def appear(pub_sub, content, state):
nonlocal visible
content.dungeon.announce(f'{name} has appeared!')
pub_sub.publish('state_number', 'x', content=self, state=state)
visible = True
Note that I used the new pub_sub parameter to do the necessary publish.
All tests are green. Let’s run the game and look for exceptions. Might help. Doesn’t help much but identifies an issue in DungeonView. I think we’ll just search to find them. Here are two:
class DungeonView:
def subscribe_to_remove_content(self, dungeon):
def callback(*, pub_sub, content):
self.view_do(content, lambda view: view.remove_from_sprite_lists())
dungeon.subscribe('remove_content', '', callback)
def subscribe_to_state_number(self, dungeon):
def callback(*, pub_sub, content, state):
self.view_do(content, lambda view: view.set_state(state))
dungeon.subscribe('state_number', '', callback)
I think I’d like this whole thing better with required keyword arguments in subscribe. Might look into that.
A search is telling me that we have them all. Somehow I doubt this. Run the game. I found one more: I had searched for ‘subscribe(‘, which doesn’t find ‘subscribe_once’. Found those and fixed. I exercised the game fully and it all worked as intended. I am comfortable enough to commit: All subscribe callbacks receive pub_sub parameter.
Now lets bind the subscriptions that CombinedContent sets up to the instance. That promises to be … interesting. Here’s where we do all our subscriptions:
class CombinedContent:
def run(self, dungeon):
self.dungeon = dungeon
self.dungeon.subscribe_all(self.subs)
The self.subs list is a list of instances of:
class Subscription:
def __init__(self, *, event, caller_id, callback, one_shot=False):
self.event = event
self.caller_id = caller_id
self.callback = callback
self.one_shot = one_shot
That class presently has no methods. Let’s do what has to be done as a method on subscription. Wishful Thinking:
Wait, I seem to have screwed up. A test is failing and I didn’t notice. Ah. Didn’t matter but there was a test dummy that wasn’t providing a PubSub in a fake callback call. Meh. Fix, commit.
Now it turns out that we don’t even need wishful thinking to do this:
def run(self, dungeon):
self.dungeon = dungeon
for sub in self.subs:
sub.callback = types.MethodType(sub.callback, self)
self.dungeon.subscribe_all(self.subs)
Hm this breaks things. Oh, and it should, since now our CombinedContent callbacks need self.
def test_subscriptions(self):
count = 0
def increment(self, *, pub_sub, content, state):
nonlocal count
print('counting')
count += 1
sub = Subscription(event='do_it', caller_id='', callback=increment)
content = CombinedContent(
name='item',
resources = [],
scale=1,
subs=[sub]
)
content.run(self)
self.pub_sub.publish('do_it',
caller_id='xxx',
content=self,
state=1)
assert count == 1
Changed the third line up there to include self,.
Same thin in this test:
def test_appearing(self):
name = 'a thing'
visible = False
def interaction(self, interactor):
nonlocal visible
if visible:
interactor.receive_content(self)
interactor.announce(f'You have received {self.name}!')
return True
def appear(self, pub_sub, content, state):
nonlocal visible
content.dungeon.announce(f'{name} has appeared!')
pub_sub.publish('state_number', 'x', content=self, state=state)
visible = True
sub = Subscription(event='state_number',
caller_id=name,
callback=appear,
one_shot=True)
item = CombinedContent(name=name,
resources=['a', 'b'],
scale=0.5,
interaction=interaction,
subs=[sub]
)
item.run(self)
# item.interact_with_player(self)
assert self.message is None
self.pub_sub.publish('state_number', name, content=item, state=1)
assert self.message == 'a thing has appeared!'
Changed the appear callback to expect self.
Commit: CombinedContent callbacks receive self as well as pub_sub, with self as the CombinedContent instance.
Reflection
Small as it seems (and easy as it was), this is kind of a big deal. The subscription change was sweeping, but it provides the ability for any callback to issue further publications or do new subscriptions. (Curiously, there is no unsubscribe. Kind of like letting a political party know your phone number. There is subscribe_once. So far I haven’t wanted unsubscribe badly enough to implement it: it will be a bit difficult.)
With this change in place, we can do announcements during callbacks without reference to the dungeon or any hacks elsewhere. Once we implement announcements via publish, we’ll be able to remove the dungeon-saving hack from CombinedContent’s run. Soon, soon.
Making the callbacks into methods on the item is admittedly a bit deep in the bag of tricks, but the magic is buried inside that class and in use, I think it will feel quite natural.
Let’s see about creating an announcement event.
Even though announcements are currently saved in Dungeon, I think we’ll do the subscription in DungeonView, which already does some subscribing.
I’ll spare you the changes, because it got a bit ragged and a couple of tests need revision because they are using a fake dungeon to check things. I think we’ll revert to the preceding commit and sum up.
Summary
Main thing: when work starts getting ragged and I feel that I’m scrambling, it’s time to reset –hard and take a break. No good can come of slamming things around to make it run.
I do think that it’ll be a good plan to publish announcements, and it was working despite some broken tests. There area a couple of things to look at more carefully when next we undertake to make that change.
One is that the interact_with_player callback in CombinedContent is unique in that it is passed the dungeon, and the other callbacks see the PubSub instance. I believe that once we get everyone running on the new CombinedContent, we should normalize that situation somehow, because writing callbacks in two different styles can only lead to trouble.
Another is to proceed a bit more judiciously and pay more attention to the couple of tests that fail. I’m pretty sure that the issue is that they use some fake dungeons and pubsubs and such and that those were not updated to the new protocol. I’d bet $10 on that but I wouldn’t bet a thousand.
But we have made good progress this morning (and early afternoon, I got a very late start). We’ve ensured that every subscription callback has access to the ability to publish and further subscribe as needed, and we’ve given the callbacks in CombinedContent access to the content instance, which will provide access to extra instance variables that the more exotic items will require.
The first change was system-wide, though we could have done it over time had we needed to. The second change is simple, just wrapping the callback to make it a method, but uses a somewhat deep Python capability. It’s public and approved, mind you. It’s just conceptually deep and something that one very rarely needs.
So both these changes, though small in code, are substantial in importance. I’m feeling good about how things are coming together and still expect that we’ll be able to do all our content using a factory and a single simple class. We may actually discover that the class is too simple. We’ll find out.
See you next time!