sphinx icon indicating copy to clipboard operation
sphinx copied to clipboard

revisit type annotations for `add_config_value`

Open danieleades opened this issue 3 years ago • 0 comments

the typings for sphinx.application.Sphinx.add_config_value are a little inconsistent.

the type signature is currently

def add_config_value(self, name: str, default: Any, rebuild: Union[bool, str], types: Any = ()) -> None:
    ...

but throughout the code base it is called using rebuild=None. This works, because None is falsey, but doesn't align with the method signature (addressed in #10681).

Then the doc comment says that the str can only have the values of "env", "html", or "", which implies

def add_config_value(self, name: str, default: Any, rebuild: Literal["", "env", "html"], types: Any = ()) -> None:
    ...

for backwards compatibility (at least until the next major change), this probably needs to be

def add_config_value(self, name: str, default: Any, rebuild: Union[bool, Literal["", "env", "html"]], types: Any = ()) -> None:
    ...

Personally, if i'd designed this I probably would have wanted to use an Enum, as that would be the most difficult to misuse.

Parking this ticket here for discussion before I propose any changes, because there's a couple of different ways to go on this one. See this thread for context - https://github.com/sphinx-doc/sphinx/pull/10681#discussion_r928152914

danieleades avatar Jul 29 '22 08:07 danieleades