ENB: mypy annotations: lessons learned

[Edward K. Ream]

Last week, I merged 7 PRs that improved mypy annotations in Leo's codebase. Before that, I closed 6 ill-considered PRs whose purpose was also to improve mypy annotations. This Engineering Notebook post discusses the lessons learned. It will be pre-writing for a new info item about annotations.

Background

I make no apologies for the six closed PRs. True, I eventually abandoned the preliminary work, but that work was essential. It deepened my understanding of mypy and made me aware of unforeseen challenges and complications. I could not have made progress without these initial missteps!

In retrospect, complaints about the abandoned PRs were partially valid, but not for the reasons given. Yes, the PRs were too large, but as I shall explain, their size was not their real defect.

New awareness

The most important takeaways from the PRs, both merged and abandoned:

- Be wary of making any changes to executable code.

- Changing the default value of any argument to a function or method is a significant change to Leo's executable code!

- PRs can change thousands of lines of code safely provided they only change the annotation of vars, ivars, args, and kwargs.

- The best PRs have the smallest possible purpose. Their size is secondary, almost irrelevant.

An alternative history

This section describes how I might have structured the mypy PRs had I been prescient, that is, had I already known what I know now. Think of this retrospective strategy as alternative history. This alternative history should help explain why what I now know makes a difference.

The next sections describe what I would do if I could start from scratch.

Update mypy's Union syntax

Replace Union [a, b] with  a | b.

This would change none of Leo's executable code, except for removing the symbol "Union" from the list of symbols imported from Python's typing module.

A single PR could safely make this change in all of Leo's codebase. Why? Because the PR's purpose is limited and completely safe.

Require strict annotation in 5 key Leo files

This step would have been similar to the five merged PRs. There was a separate PR for leoNodes.py, leoGlobals.py, leoCommands.py, and finally leoApp.py, and leoConfig.py. The order of the PRs is natural. First, annotate p and v, then g, then c, and finally g.app and g.app.config.

In retrospect, these PRs should have minimized changes to executable code. In particular, these PRs should not have changed any default values!

Anyway, the merged PRs had to change quite a bit of executable code to please mypy. mypy's casts were sometimes essential, and using casts implied changing imports and adding asserts. The details are interesting, but I'll skip them here.

Imo, these PRs would have been quite safe, despite their size, but only if they had not changed the defaults for kwargs. Alas, I wasn't so wise, so I have a lot of checking to do.

Now we move from alternative history to future history.

Replace tests on None

This PR is dangerous. It will replace:

if x is None:

if x is not None:

with:

if not x:

if x:

respectively. I've discussed such changes before. All such changes could break code. The PR must ensure that such bugs never happen. A lot of testing will be required.

Replace defaults for strings

In the alternative history, I would have waited until the last few PRs before changing s: str | None = None to s: str = ''.

But, big sigh, several merged PRs have already made such changes. That was a mistake that must be managed. All methods containing such changes must be checked! As above, I will want to change all string tests from:

if s is None:

if s is not None:

to:

if not s:

if s:

Aha! mypy doesn't know about empty strings

This insight was quite a shock. It shouldn't have been: mypy is a type checker, not a value checker. Perhaps that is why I so recklessly changed defaults.

Summary

The distinction between alternative history and future history is only a rough approximation of the work to be done. Leo's codebase is a mish-mash at present. The coming PRs must make Leo's codebase look as if I had followed a prescient strategy.

Changing default values for kwargs prematurely was a big mistake. Now, I must guarantee that none of those changes will introduce bugs. There is an interplay between changing defaults and using pythonic tests. But as always, pythonic tests are no guarantee of error-free code.

The best PRs have a single, well-defined purpose. The size of the resulting PRs hardly matters.

All questions, comments, and suggestions are welcome.

Edward