Python Asteroids+Invaders on GitHub

Today, I plan to install the new metaclass with the ignore feature. Result: Bears discovered deep in the bag of tricks. They bite. Later: I know what caused it.

Refresher

The new class IgnoreThese is a metaclass that enables us to write class definitions like this:

class FooIgnores(Base, metaclass=IgnoreThese, ignore=["bar", "baz"]):

That class inherits from Base, and will be set up so that methods bar and baz are null. They will do nothing, and return None. We’ll be using this metaclass to reduce the size and clutter in our Flyer subclasses, which currently either need to implement each of the would-be ignored methods as an explicit pass, or to inherit concrete implementations from their superclass. We are not deeply dissatisfied with the inheritance, although many advisors would advise against the practice. But even if we are comfortable with the inheritance, the new notation is more explicit about what is being ignored.

Prior to today, we had this list of pros and cons for use of this new metaclass:

Pro
Code today is cluttered with empty methods. In some cases, half of a class is empty methods. This scheme would reduce that to a simple list of names in the class definition.

The question of what a class pays attention to would be somewhat easier to work out, both because the class will be smaller, and because if the method does not appear in the ignore list, the class must process it.

In short, our code will be easier to write, shorter, and easier to understand.

It seems very unlikely to fail. We would welcome more tests to try to make it fail.

Even if it did fail, we do not see a way in which it could fail that would result in a defect in the program. It could make the program fail to compile, but we don’t see much chance of a covert error.

Con
This is quite deep in the bag of tricks. It is tested well enough that we can be sure that it works, but no one on the team really understands metaclasses well enough to fully understand this.

It’s clever. Clever code should be avoided.

It’s weird and scary and scary weird.

I would say that the arguments against come down to the approach being pretty deep in our bag of tricks, deep enough that we don’t have the breadth of understanding of metaclasses that we like to have when we use things. In favor, the code will be easier to write and to understand.

There is a reliability question that should have gone into the pros and cons.

If we implement all the required methods as abstract, then PyCharm and the compiler will detect any object that doesn’t implement all of them. That’s about as solid as it can get. However, our coding standard currently allows a team (me) to implement required methods as concrete, and null: pass. That allows individual classes not to implement all the many methods, at a small cost in reliability, in that we might forget one that needed an implementation and whatever that feature was just wouldn’t be there.

In the metaclass scheme, we plan to implement all the required methods as NotImplementedError in the superclass, so that each subclass will have to implement the method or ignore it. Unfortunately, if that’s all we do, we’ll be subject to a run-time error in the error case, not a compile-time error. That’s not quite as good.

A test comes to mind. If we were to assemble all the objects inheriting from, say, InvadersFlyer, into one is, and interact them, every object would be sent every possible interact_with_* message and any object that was not hooked up correctly would assert. That’s such a good idea that we should have done it ages ago. With that in place, the new scheme is pretty safe.

I’m glad we had this little chat.

Anyway, those are the pros and cons that I see going in. I expect that more may arise as we put this scheme in place.

Planning

Here are some of the things that need to be done:

  1. Move IgnoreThis class to prod side.
  2. Write the test described above. I had been thinking of it as done later, but it may well help during conversion to the metaclass.
  3. Implement required methods in superclass as NotImplementedError. Probably should do them one at a time.
  4. In the classes that fail the test, figure out the ignores required. Could do this one method at a time but it may make more sense to convert each class only once.

I think it’s nearly that easy. Actual results may vary.

My tentative plan is to do one of the conversions by hand, to get a feeling for it before getting too invested.

Installing IgnoreThese

I move the IgnoreThese class out of the testing tree without difficulty. Commit: move IgnoreThese to prod.

At this moment I sincerely wish that I had developed the practice of keeping my various classes in subfolders. The tests are in a subfolder of the project, but all the production files are in one big mess. That’ll make it harder to decide who to fix up first.

I let chance guide me. The Bumper class happens to be open in one of my tabs. It’s a lovely candidate, because it basically interacts with no one.

Long source deleted, since the effort fails anyway.

The new Bumper class definition is this:

class Bumper(InvadersFlyer, metaclass=IgnoreThese, ignore=
["interact_with_bumper",
 "interact_with_invaderexplosion",
 "interact_with_invaderfleet",
 "interact_with_invaderplayer",
 "interact_with_invadershot",
 "interact_with_playerexplosion",
 "interact_with_shield",
 "interact_with_playershot",
 "interact_with_shotcontroller",
 "interact_with_shotexplosion",
 "interact_with_topbumper"]):

And all the tests fail. Well, five fail and 196 don’t even run. Top level collect failed.

E   TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases

I do not know what that means. I’ll ask the internet. I do not understand what the internet is telling me: it is mostly talking about multiple inheritance and metaclasses. I try, briefly, having Flyer inherit from IgnoreThese. That lets more tests run but not all of them, with a new strange error about __new__ not having a classdict.

I roll back.

Reflection

The metaclass scheme is now seriously compromised, since right out of the box it broke in a way that I do not understand. We obviously cannot go ahead with it, at least until we do understand it, and possibly we should conclude right here and now that it’s too deep in the bag to be used.

We do have the less bizarre version that used a call to super() to do the ignores. If I recall, that one may not allow for overriding ignored items with actual methods.

So. First attempt to use the metaclass fails. We have other work to do and should continue, but in my copious free time, I plan to learn more about this. The first step would be to duplicate the error in a test but if I do, I think it’ll break all the tests again.

Summary

Debacle. Rolled back. As much as I like the idea of automating the methods to be ignored, the only real payoff is reduction of empty methods in subclasses, which we can provide at a fairly low risk by making some of the required methods non-abstract.

For my own education, I may explore this idea and metaclasses further. But for now, this idea, despite what a nice feature it promised to provide, is dead in the water.

So be it. Sometimes the bear bites you. Apparently there are bears deep in the bag of tricks. Who knew? We all did!

See you next time!

Later that day …

The cause of the problem is the use of ABC, the superclass that provides abstract methods. ABC uses its own metaclass, ABCMeta. I could almost certainly make IgnoreThese work by removing all the abstract class stuff and using our own metaclass instead.

I also tried making my metaclass inherit from ABC and type, but that didn’t work. Making IgnoreThese inherit ABCMeta and my base class inherit ABC does seem to work. So I think I could try using the new scheme with my class inheriting ABCMeta. However, I have no idea what the implications of that might be. Seems far too risky.

Of the two alternatives, stopping the use of ABC in the Flyer hierarchy seems more reasonable, but would replace a strong mechanism that is part of Python with a weaker mechanism that is hand-rolled. Since I clearly do not fully understand the metaclass machinery, this does not seem like a good trade-off. Investigation may continue.