Menu

#210 Typing and `docutils.nodes.Node`

None
open
nobody
None
5
2024-08-15
2024-08-15
Adam Turner
No

Commit [r9901] by @milde changes instances of Element | Text to Node, and adds attributes to Node. I think that this is a not the best and the commit should be reverted before the impending 0.22 release, to allow for more discussion.

As I noted in commit [r9813] which added type hints:

Add type hints to ``docutils.nodes``

A significant number of parameters and attributes are typed as
expecting or yielding ``Element | Text`` rather than ``Node``,
which is the more obvious choice. This union type is used because
``Node`` is de facto an abstract base class --- it should not
be used or instantiated. Notably, ``Node.children`` is not defined,
``Node.attributes`` does not exist, etc. The two direct subclasses
fill in many of these missing pieces, making static typing both
more precise and easier.

Element and Text are different types, as Text can be treated as a string and Element means that we have a concrete node type with attributes and data. Conflating these as Node is unhelpful, and we shouldn't signal to users that a Node is expected anywhere, as whilst is is de facto an abstract base class, it can be instantiated and used at runtime, which should be heavily discouraged.

Aside from verbosity, I'm unsure of the argument to change this. Verbosity can be managed with a type alias, as we had previously used (ElementT, though this should perhaps be named ElementOrText).

A

Related

Commit: [r9813]
Commit: [r9901]

Discussion

  • Adam  Turner

    Adam Turner - 2024-08-15
    • Description has changed:

    Diff:

    --- old
    +++ new
    @@ -2,7 +2,7 @@
    
     As I noted in commit [r9813] which added type hints:
    
    -```
    +```text
     Add type hints to ``docutils.nodes``
    
     A significant number of parameters and attributes are typed as
    
     

    Related

    Commit: [r9813]

  • Günter Milde

    Günter Milde - 2024-08-15

    Aside from verbosity, I'm unsure of the argument to change this.

    Readabilty counts.

    Verbosity can be managed with a type alias,

    This was my first idea. The best name for such a type alias would be "Node" (for any node in a document tree). The name was already take by a class documented as "Abstract base class of nodes in a document tree."
    So, instead [r9901] makes sure that all methods and attributes that are common to "Element" and "Text" are also defined for "Node": Node.children is now an empty tuple (as in Text).
    There is still no "Node.attributes". However, if a return value "rv" is hinted as "Element | Text" "rv.attributes" cannot be safely used either (as "Text.attributes" does not exist).

    To safequard against accidential use of a "Node" instance, "Node" can be based on "abc.ABC" and the abstact methods decorated with "abc.abstractmethod". See appended patch.

     

    Related

    Commit: [r9901]


Log in to post a comment.