[lint] run mypy on tests/test_config/test_config.py

[picnixz]

When implementing #12196, I wanted to use mypy on it. So I did that. However, I observed that we are facing some issues (that I didn't think of before...)

• Patched objects (i.e., those with mock.patch) are NonCallableMock objects. However, typing them as such makes PyCharm lose it's autocompletion... On the other hand, typing them as Any make mypy lose its purpose... (and note that NonCallableMock inherits from Any in typeshed).
• The Config object does not support __setattr__ explicitly, although it should, making mypy raise a lot of attr-defined errors. This only happens if you type app as Sphinx. So... I'm not really sure we are gaining anything in the future if we need to either add an explicit __setattr__ or if we need to add # type: ignore everywhere...

I think we need to delay #12097 until we have a good idea on how to minimize the introduction of # type: ignore. The only good idea I can come up with, without destroying both autocompletion and mypy at the same time, is to implement a mypy plugin which would allow us bypassing type annotations at the level of the test itself. (plugin wouldn't solve the issue).

I had the habit to ask people to either not type or type everything in the function. However, with the limitations by mypy and IDEs, I feel we need to allow incomplete definitions (for tests at least).

cc @danieleades @chrisjsewell @jayaddison

[jayaddison]

Suggetion: we could try to unpick this a bit and make the interface, and then test case, cleaner. Perhaps correct_copyright_year should only accept a single argument, the app to check/correct copyright for?

[chrisjsewell]

I had the habit to ask people to either not type or type everything in the function. However, with the limitations by mypy and IDEs, I feel we need to allow incomplete definitions (for tests at least).

yes, I would certainly suggest enforcing types less strictly in tests, since obviously these are only for our internal use rather than anything exposed to the public, and we don't want to making writing test to much of a burden 😅

[danieleades]

• The Config object does not support __setattr__ explicitly, although it should, making mypy raise a lot of attr-defined errors. This only happens if you type app as Sphinx. So... I'm not really sure we are gaining anything in the future if we need to either add an explicit __setattr__ or if we need to add # type: ignore everywhere...

i think adding and assuming the existence of properties on a foreign config object is a pattern which is fundamentally opposed to strict type checking.

i presume adding __getattr__ and __setattr__ won't remove all the ignores because they'll return Any...

A better mechanism (though a much larger architectural change) might be for plugins to somehow subclass Config and add their own properties that way

[picnixz]

i presume adding getattr and setattr won't remove all the ignores because they'll return Any...

__getattr__ already exists but not __setattr__ actually (and yes the first returns Any as it should be)

A better mechanism (though a much larger architectural change) might be for plugins to somehow subclass Config and add their own properties that way

This one is feasible, but there is an issue with the fact that configuration objects behave as Any but raise an AttributeError upon non-existing attributes. However, the attributes are not known at compilation time since they are dynamically added via config.add (which does not require a literal string...). So, a plugin would "half-solve" the problem if the configuration values are literal strings.

If you want, it's like a types.SimpleNamespace object but with dynamic set of allowed properties. And honestly, the best typing for that is... Any (+ explicit interface on Config objects).

[chrisjsewell]

A better mechanism (though a much larger architectural change) might be for plugins to somehow subclass Config and add their own properties that way
So, a plugin would "half-solve" the problem if the configuration values are literal strings.

Yeh i think these are beyond the scope of this PR,
but what I can tell you is that in myst-parser and sphinx-needs, I utilise a dataclass as the "source of truth" for extension configuration (with type checking): https://github.com/executablebooks/MyST-Parser/blob/dfaf6d7abfdd988c74a1d6e1bbdc637e9f5b5bcb/myst_parser/config/main.py#L181

Then I have a layer on top of sphinx to:

1. call app.add_config_value for all variables
2. add config value type validation (in a config-inited event)
3. create the dataclass instance from the app.config, to access the variables
4. Also have a custom directive that auto-generates the config documentation, ensuring it is always in-sync with the codebase 😄 : https://myst-parser.readthedocs.io/en/latest/configuration.html

I'd be intersting if any of this, at least conceptually, could be upstreamed to sphinx core

[picnixz]

but what I can tell you is that in myst-parser and sphinx-needs, I utilise a dataclass as the "source of truth" for extension configuration (with type checking):

From what I understood, extensions would write a dataclass as their "additional configuration" and you would merge them together right, producing a "single" configuration class that inherits from those dataclasses? While this logic could work, since Python does not have an intersection type (unlike TypeScript), mypy would not be able to tell that the configuration object after adding multiple subconfigurations dataclasses supports all those fields (unless we do a plugin I think and fake an inheritance).

Currently, in Sphinx, the simple fact that you can add configuration values however you see fit makes Config objects impossible (from a static point of view) to lint them.

[chrisjsewell]

From what I understood, extensions would write a dataclass as their "additional configuration" and you would merge them together right, producing a "single" configuration class that inherits from those dataclasses?

yep, something like that 😅
very simply, my solution still has an aspect of "type-unsafeness", but that is confined to a single point in the codebase (when you generate the MdParserConfig from the Config), then at least every other part of the code base is "type-safe"

While this logic could work ... unless we do a plugin I think and fake an inheritance

Exactly; although the code I have set up in myst-parser works for me, and I'm pretty happy with it, I'm certainly not claiming it is "strictly correct" (if that it is even possible)
Plus, it would certainly take additional work, to make it more generically applicable for sphinx core/extensions

[danieleades]

this is conceptually similar to what i do in sphinx-graph (which itself is a much, much leaner take on the sphinx-needs use-case)

(in this case i'm reading from the environment rather than config)

https://github.com/danieleades/sphinx-graph/blob/27c84c3c2e28a1e23edde2f0b564b87d0a9732c0/src/sphinx_graph/vertex/state.py#L94-L116

class State:
    """State object for Sphinx Graph vertices."""


    def __init__(
        self,
        vertices: dict[str, tuple[int, Info]],
        graph: rx.PyDiGraph[str, str | None],
    ) -> None:
        """Create a new state object."""
        self._vertices = vertices
        self._graph = graph


    @classmethod
    def read(cls, env: BuildEnvironment) -> State:
        """Read the State object for the given environment.


        This is a read-only view of the state. Changes will not be saved.
        """
        vertices = getattr(env, "graph_vertices", {})
        graph: rx.PyDiGraph[str, str | None] = getattr(
            env, "graph_graph", rx.PyDiGraph(multigraph=False)
        )
        return State(vertices, graph)

it's 'loosely-typed', but i do it only once in the codebase and use a typed interface everywhere else

[chrisjsewell]

in this case i'm reading from the environment rather than config

sphinx-needs also does a similar thing, to return extension specific state from the environment 😄

at the end of the day, this seems probably the most pragmatic approach (confining the loose typing to a single place)

I don't know if there is any generalisation to be had here, to help extension developers use the "best practices", or if you just leave them to their ad-hoc solutions 🤔

[picnixz]

I think we should wait until we have intersection types in Python (see python/typing#213). After that, it will be easier to type extendable configurations.

[electric-coder]

typing them as such makes PyCharm lose it's autocompletion

This should be a non-concern over having mypy working. Stub those objects separately if you want to keep the autocompletion working.

@picnixz

I think we should wait until we have intersection types

The only mention of Intersection is a speculation in PEP 483 soon the issue will be "under discussion" for 10 years in several assorted echo chambers without a single tangible step forward in its direction. So lets be realistic: seeing Intersection of Python types is a myth with no set timeline hidden in a mirage beyond visual range. Let's not hold our breath waiting for it...

[AA-Turner]

without a single tangible step forward in its direction. So lets be realistic: seeing Intersection of Python types is a myth

Note that TypeIs allows for intersection type behaviour.

A

[electric-coder]

Note that TypeIs allows for intersection type behaviour.

That's a surprise, PEP 742 announced in mid-February just-in-time to make the final cut in May for Python 3.13...

I spend too much time grappling with nearly decade old unfixed mypy bugs that I just can't keep up with the latest announcements. 😒

[picnixz]

Before replying I need to remember the discussion...

This should be a non-concern over having mypy working. Stub those objects separately if you want to keep the autocompletion working.

Yes but in the end, it's either that or typing them as Any, in which case it doesn't really help more. Stubbing them is an issue we need to tackle separately. MyPy would work with Any, but it wouldn't help us detecting issues. Having Any is way to broad because some operations are disallowed. What was annoying is:

@mock.patch("sphinx.config.logger")
def test_overrides_dict_str(logger: Any) -> None: ...

Ideally, I wanted to have a typed logger but since it's a mocked object, I cannot do it... so in the end I end up with an Any logger which does not help more...

For intersection types, I was probably thinking about the extensibility of Config rather than something else. However, it could have helped in the above case since I could make it as the intersection of a mock object and a logger object.

As for what to do with this PR, we either:

• add some # type-ignore just because we can't do more...
• forget about it (which I think is fine)

[AA-Turner]

Closing, as this PR is now very out of date with several merge conflicts -- we are gradually adopting type hints in the tests though.

A

[picnixz]

I'll rewrite it in the future (better than just fixing conflicts).