-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add static types for the HTML context dict #13824
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
9b27ad8
to
2291028
Compare
@@ -416,7 +415,7 @@ def test_html_style(app: SphinxTestApp) -> None: | |||
}, | |||
) | |||
def test_html_sidebar(app: SphinxTestApp) -> None: | |||
ctx: dict[str, Any] = {} | |||
ctx: _PageContextHTML = {} # type: ignore[typeddict-item] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this test app.builder.add_sidebars
takes as its ctx
parameter an empty dictionary - is that a valid use of app.builder.add_sidebars
?
If so, it seems as though the type hint of app.builder.add_sidebars
's ctx
parameter should change.
If not, it seems as though this test should change to be realistic by actually having ctx
be a _PageContextHTML
rather than an empty dictionary.
Wonderful to see this. First minor nit: the "interface" is defined in kind of a buried way, in |
Related: some of the items in there are likely beyond the HTML builder. Some of the items are global to all pages and come from config. My guess: you should have one "interface" for the configuration system, another for stuff unique to each doc's build, and a third for parts unique to the HTML builder. As such, perhaps start a new place called Which then means..more work. 😉 You might make these contracts as protocols, then have the HTML builder construct an object that says it is one of those. I'm 50/50 on that. It is duplication. Your approach of |
self.globalcontext |= { # type: ignore[typeddict-item] | ||
f'theme_{key}': val | ||
for key, val in self.theme.get_options(self.theme_options).items() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to indicate globalcontext
as "this fixed set of keys, plus zero or more arbitrary others of str -> Any
" -- e.g. we add these theme_{...}
keys, the self.config.html_context
mapping on the next line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the juice isn't worth the squeeze - something like the following code might do it but that is a mess. I wonder - could you separate something like base_context
and extra_context
variables - meaning there is always type checking for the base_context
. I haven't looked into the details of where this is used and what trade-offs are there.
class _GlobalContextHTML(Protocol):
def __delitem__(self, key: str) -> None: ...
def __iter__(self) -> Iterator[str]: ...
def __len__(self) -> int: ...
def items(self) -> Iterable[tuple[str, Any]]: ...
def copy(self) -> Self: ...
def __or__(self, other: dict[str, Any]) -> dict[str, Any]: ...
def __ror__(self, other: dict[str, Any]) -> dict[str, Any]: ...
def __ior__(self, other: dict[str, Any]) -> Self: ...
def get(self, key: str, default: Any = ...) -> Any: ...
def update(self, other: dict[str, Any]) -> None: ...
@overload
def __getitem__(self, key: Literal['embedded']) -> bool: ...
@overload
def __getitem__(self, key: Literal['project']) -> str: ...
@overload
def __getitem__(self, key: Literal['release']) -> str: ...
@overload
def __getitem__(self, key: Literal['version']) -> str: ...
@overload
def __getitem__(self, key: Literal['last_updated']) -> str | None: ...
@overload
def __getitem__(self, key: Literal['copyright']) -> str: ...
@overload
def __getitem__(self, key: Literal['master_doc']) -> str: ...
@overload
def __getitem__(self, key: Literal['root_doc']) -> str: ...
@overload
def __getitem__(self, key: Literal['use_opensearch']) -> bool: ...
@overload
def __getitem__(self, key: Literal['docstitle']) -> str | None: ...
@overload
def __getitem__(self, key: Literal['shorttitle']) -> str: ...
@overload
def __getitem__(self, key: Literal['show_copyright']) -> bool: ...
@overload
def __getitem__(self, key: Literal['show_search_summary']) -> bool: ...
@overload
def __getitem__(self, key: Literal['show_sphinx']) -> bool: ...
@overload
def __getitem__(self, key: Literal['has_source']) -> bool: ...
@overload
def __getitem__(self, key: Literal['show_source']) -> bool: ...
@overload
def __getitem__(self, key: Literal['sourcelink_suffix']) -> str: ...
@overload
def __getitem__(self, key: Literal['file_suffix']) -> str: ...
@overload
def __getitem__(self, key: Literal['link_suffix']) -> str: ...
@overload
def __getitem__(self, key: Literal['script_files']) -> Sequence[_JavaScript]: ...
@overload
def __getitem__(self, key: Literal['language']) -> str | None: ...
@overload
def __getitem__(self, key: Literal['css_files']) -> Sequence[_CascadingStyleSheet]: ...
@overload
def __getitem__(self, key: Literal['sphinx_version']) -> str: ...
@overload
def __getitem__(self, key: Literal['sphinx_version_tuple']) -> tuple[int, int, int, str, int]: ...
@overload
def __getitem__(self, key: Literal['docutils_version_info']) -> tuple[int, int, int, str, int]: ...
@overload
def __getitem__(self, key: Literal['styles']) -> Sequence[str]: ...
@overload
def __getitem__(self, key: Literal['rellinks']) -> Sequence[tuple[str, str, str, str]]: ...
@overload
def __getitem__(self, key: Literal['builder']) -> str: ...
@overload
def __getitem__(self, key: Literal['parents']) -> Sequence[_NavigationRelation]: ...
@overload
def __getitem__(self, key: Literal['logo_url']) -> str: ...
@overload
def __getitem__(self, key: Literal['logo_alt']) -> str: ...
@overload
def __getitem__(self, key: Literal['favicon_url']) -> str: ...
@overload
def __getitem__(self, key: Literal['html5_doctype']) -> Literal[True]: ...
@overload
def __getitem__(self, key: str) -> Any: ...
def __setitem__(self, key: str, value: Any) -> None: ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://peps.python.org/pep-0728/ - this is approved and will solve for the case here, I believe.
Purpose
Improves static typing for the various
ctx
/addctx
/globalcontext
dicts that we use in the HTML builder.References