This Engineering Notebook post discusses a recent change to one of the Position class's fundamental low-level methods: p._parentVnode.
This change is now part of PR
#4767. I believe the change is necessary, but I want to discuss my thoughts publicly.
tl;dr: p._parentVnode must raise ValueError when p has no valid parent VNode. Doing so is pythonic and helps prevent data loss. It also simplifies the method's return annotation.
Background
mypy rightly drew my attention to p._parentVnode. It is annotated to return a VNode, yet the legacy version of the routine may also return None!
More than a dozen of Leo's core methods call p._parentVnode, including four low-level position methods. None of those low-level methods check the value returned by p._parentVnode!!
This creates a mystery: why hasn't Leo been crashing?
First thoughts
This mystery absolutely must not be ignored.
Papering over the mystery, say by fudging p._parentVnode's annotated return value, would be a dereliction of duty. The point of the strict_optional mypy setting isn't prissy cleanliness. It's a deeper understanding of Leo's code, especially the Position and VNode classes.
Traces show that p._parentVnode never returns None, which is a pragmatic explanation of why Leo doesn't routinely crash. But relying on that kind of explanation amounts to relying on hope. Something more rigorous is required.
My first thought was that I needed a proof that p._parentVnode never returns None. But where to start? There are too many callers within Leo itself.
The solution
After long thought, I think that p._parentVnode must raise ValueError in the following unusual situations:
- p is empty (p.v is None).
- p.v.context is empty (p is unattached to any outline).
- p.v is already c.v.context.hiddenRootNode.
Raising ValueError is best for several reasons:
- ValueError is an accurate description of the erroneous situations listed above.
- Raising some kind of exception is the best way to prevent data loss.
Laboriously testing the results of p._parentVnode would be unpythonic. The crucial low-level callers would have to raise another exception anyway! They would have no other way to ensure that their callers would handle the error correctly!
Summary
It would be intolerable to leave p._parentVnode as it was.
Raising ValueError is the best way to handle unexpected (invalid) arguments to p._parentVnode.
Raising ValueError also informs mypy that p._parentVnode expects to return only (valid) VNodes.
This new version of p._parentVnode joins the short list of actual code changes in PR
#4767.
All of your comments, questions, and suggestions are welcome.
Edward