On Mar 2, 2021, at 6:10 PM, Jean-Paul Calderone <exarkun@twistedmatrix.com> wrote:

Policy aside, this change doesn't seem like much of an improvement to me.  If I were to guess, I would guess the change was made to satisfy some check Mypy is now being asked to make about Twisted.  If that's the case, it seems unfortunate that real-world software is suffering so that a synthetic goal can be achieved.  I do recognize there is a perception that practical value can come from attending to the errors Mypy reports.  It would probably benefit everyone if more care were taken to consider the real-world consequences of changes that are made to satisfy the non-real-world goalposts set by tools like Mypy.

I do think that Mypy was likely highlighting a real issue here, and it should have been fixed.  We had documented interfaces already, and we were failing to adhere to them properly, and Mypy was complaining about that.

For easy reference, the change that made these fixes is here https://github.com/twisted/twisted/pull/1298

This genre of fix is definitely something we should unwind over time, although it does make it easier to start mypy-clean rather than have hundreds of exclusion lines that need to be manually adjusted, so on balance I agree with this tradeoff at the point it was made.

Specifically what I mean by "this genre of fix" is that you can always make mypy happy by transforming code like this:

class Thing:
    pass

Thing().stuff()

into code like this:

class Thing:
    @property
    def stuff(self):
        raise AttributeError("no such thing as `stuff`")

Thing().stuff()

The runtime behavior is the same, but now Mypy can't tell you about this class of bug any more.

So, at some point, we should slowly unwind all `NotImplementedError` methods and find ways to either implement them for real or make their required use raise at import time.

Finally: let's not develop an aversion to new tooling and change because it might create problems; experience over the last few years has shown me that Mypy can catch tons of real bugs and it's well worth getting the codebase to type check.  If we want to prevent breakages like this in the future, the answer is not to stop trying to get linters and typecheckers to run cleanly with arbitrary changes, but to figure out some kind of continuous integration solution that's actually continuous with our downstream dependencies

If dependencies could start testing against Twisted trunk in some capacity, we could get notified close to when unintentionally breaking changes occur, and dependencies can let us know well before the release happens, and we can either revert or they can fix things if the error is on their end.  If initially, say, crossbar and matrix would like to work with us to set up some kind of repeatable pattern we can suggest to others, that would be great.

-g