ENB: I've closed PR #4755: lessons learned and new plans

[Edward K. Ream]

Here are two quotes from Thomas Edison:

"I have not failed. I've just found 10,000 ways that won't work."

"There are no rules here--we're trying to accomplish something."

PR #4755 contains valuable nuggets. Nevertheless, it is fatally misguided. I have just closed it. This Engineering Notebook post discusses what I learned, what I'll retain, and my new strategy. I'll create a new PR later today or early tomorrow.

But first, a big thank you to Thomas. His critiques finally got through to me.

Why the PR is misguided

This morning, I spent several hours looking for "if" statements that would have to change (within Leo) as a result of the PR. At last I realized: Doh! The PR makes massive changes to Leo's API. That would be intolerable: everyone's (private) scripts, including @button nodes, and plugins could easily break. And not necessarily in easy-to-correct ways. Game over.

What will endure

The old PR uncovered several real or potential bugs. They are all marked with the "# strict_optional" comment.

Similarly, the old PR highlights local style improvements that have no global implications. In particular, I'll retain improvements that help mypy infer the correct type. One such pattern is replacing:

for key in sorted(d.keys()):

   val = d.get(key)

   ...

with:

for key, val in sorted(d.items()):

   ...

These two patterns are equivalent, but mypy only fully understands the second.

A cff combined with an outline copy will retain the relevant nodes for the next PR.

Improving Leo's API without breaking code

This morning, I realized that it is possible to change the signature of two crucial methods, c.currentPosition and c.rootPosition, without affecting any valid Leonine script or plugin.

At present, these two methods are annotated as returning a Position. But this is a big fat lie! In some cases (which actually happen), they can return None! Each contains a mypy suppression to hide the lie.

We have all been able to get away with this intolerable situation because the only proper ways to test whether a position is "real" are with either one of the following:

   if p:

   if not p:

These patterns are clearly documented. And it's clearly documented that the following tests are invalid:

   if p == None:

   if p != None:

So here's the trick: Instead of returning None, the two methods could return a Position p with p.v == None. The p.__eq__ and p.__ne__ methods already enforce the valid patterns above.

In short, it's possible to change c.currentPosition and c.rootPosition so that:

- They always return a (possibly empty) Position.

- All valid existing scripts and plugins will continue to work.

mypy command-line options

In this project, knowing what not to do is more valuable than knowing what to do:

There is (probably) no need to use the "follow imports" option: During development, I'll always use the --follow-imports=skip option.

There will be a series of PRs: each containing a fixed set of Leo's core files. Echoing Edison, I'll try to keep each PR "clean" without dragging in other files.

A trick suggests itself: use the  --config-file setting to switch between a testing configuration (say .mypy-test.ini)  and Leo's legacy configuration, .mypy.ini.

Switching the config files will be surprisingly important. There should be no need to change more than one file at a time. In the limit, PRs could change only one file, but I'll probably have each PR change five or 10 files.

The prime directive

Leo's API must remain functionally unchanged. All (valid) scripts and plugins must work exactly as before.

Happily, following this directive is easier than violating it! Easy regex searches will suffice.  The general pattern is:

replace x: aType = None with x: aType | None = None.

Just that much should allow Leo's codebase to pass with mypy strict_optional setting set to True. Mission Accomplished!

Yes, there are a few complications. Context matters. The pattern shown above should work in all function definitions, including argument lists and return values.

In contrast, casts may be needed in ctors and other similar functions. The old PR has given me a good feel for such subtleties. mypy is remarkably flexible in this regard, and mypy, flake8, or pylint appear to catch any mistakes in this area.

Summary

The old PR did way too much and destroyed Leo's API. There is no way to redeem it. I must start afresh.

However, a cff (followed by an outline copy of the "good parts") will help me keep track of improvements that should be retained.

A series of new PRs will change a fixed (small) set of files in Leo's core, including Leo's Qt gui plugin files. These PRs will not cascade changes to other files. Manipulating mypy's config files will allow testing all files with and without the strict_optional setting. This innovation is crucial to maintaining sanity :-)

One new PR will annotate both c.currentPosition and c.rootPosition as returning (only) a Position. For the first time, this annotation will tell the truth.

I'll begin work on the first new PR soon. I expect surprises, but I am confident that the new start will eventually succeed.

All your questions, comments, and suggestions are welcome.

Edward

[Thomas Passin]

On Tuesday, June 30, 2026 at 6:13:33 PM UTC-4 Edward K. Ream wrote:

Here are two quotes from Thomas Edison:

"I have not failed. I've just found 10,000 ways that won't work."

"There are no rules here--we're trying to accomplish something."

PR #4755 contains valuable nuggets. Nevertheless, it is fatally misguided. I have just closed it. This Engineering Notebook post discusses what I learned, what I'll retain, and my new strategy. I'll create a new PR later today or early tomorrow.

But first, a big thank you to Thomas. His critiques finally got through to me.

...

One new PR will annotate both c.currentPosition and c.rootPosition as returning (only) a Position. For the first time, this annotation will tell the truth.

This is all to the good , and I have one request. Please have the PR for  the c.currentPosition and c.rootPosition changes be the only changes in its PR.

[Edward K. Ream]

On Tuesday, June 30, 2026 at 6:27:48 PM UTC-5 Thomas wrote:

>> PR #4755 contains valuable nuggets. Nevertheless, it is fatally misguided. I have just closed it.

>> One new PR will annotate both c.currentPosition and c.rootPosition as returning (only) a Position. For the first time, this annotation will tell the truth.

> Please have the PR for the c.currentPosition and c.rootPosition changes be the only changes in its PR.

Hmm. Let's call this PR zero for the project that #4766 describes. PR zero won't tell us much unless we change some mypy settings, and doing that would require improved annotations for most of the methods in leoNodes.py. Btw, I've already tested these methods (eaten my own dog food) while working on the just-closed PR. I see no particular reason to be afraid of the changes.

In any case, PR one will require strict_optional annotations in leoGlobals.py and leoNodes.py. This PR will investigate the interactions of annotations between files, so PR one must change at least two files. I'm thinking that changing c.currentPosition and c.rootPosition in PR one should be safe enough. Such changes would be the only non-annotation changes in PR one.

I've changed my mind about having two versions of .mypy.ini, the mypy settings file. That's likely a bad idea. Instead, mypy_leo.py will contain a switch that changes between "production" and "development" modes. Each new PR will change .mypy.ini so that it requires strict_optional only for Python files that contain the enhanced annotations.

Summary

Paying close attention to the typing "infrastructure" (.mypy.ini and mypy_leo.py) was an important lesson that I haven't discussed before.

Edward