On Wed, Mar 3, 2021 at 3:08 AM Glyph <glyph@twistedmatrix.com> wrote:

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.

Broadly, I agree.  But not with this part.  It seems like there is clearly a trade-off that is better for everyone.  The trade-off represented by #1298:
  • Breaks application code without providing any new functionality or fixing any bugs
  • Captures a long list of TODOs as an arbitrarily complex collection of sources spread across the entire codebase
  • All the work of addressing the problems still remains to be done 
 Contrast this with any basic ratchet-style approach (for example, capture the list of mypy errors, then allow any PR that either removes some or doesn't add any new ones):
  • At the outset, no application code breaks because nothing has actually changed
  • As work towards fixing the TODOs progresses, if it does break application code then at least applications have a chance at some benefit
  • As the work towards fixing the TODOs progresses, maintainers can easily look up what issues remain by consulting the list of errors that feeds the ratchet-style job.
The exact mechanism for the ratchet isn't the point here.  Maybe Mypy plays more nicely with inline comments telling it to ignore something, I don't know.  The point is:
  • A centralized list of stuff left to do is better than a mess smeared across the whole codebase
  • If we're going to risk breaking applications we should at least try to offer some value to them
  • We shouldn't make applications deal with every change twice when we could just disturb them once
Jean-Paul


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
_______________________________________________
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python