-
-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
from __future__ import annotations | ||
|
||
from typing import TYPE_CHECKING | ||
|
||
if TYPE_CHECKING: | ||
from collections.abc import Callable, Sequence | ||
from typing import Any, Literal, Protocol, TypedDict | ||
|
||
from sphinx.builders.html._assets import _CascadingStyleSheet, _JavaScript | ||
|
||
class _NavigationRelation(TypedDict): | ||
link: str | ||
title: str | ||
|
||
class _GlobalContextHTML(TypedDict): | ||
embedded: bool | ||
project: str | ||
release: str | ||
version: str | ||
last_updated: str | None | ||
copyright: str | ||
master_doc: str | ||
root_doc: str | ||
use_opensearch: bool | ||
docstitle: str | None | ||
shorttitle: str | ||
show_copyright: bool | ||
show_search_summary: bool | ||
show_sphinx: bool | ||
has_source: bool | ||
show_source: bool | ||
sourcelink_suffix: str | ||
file_suffix: str | ||
link_suffix: str | ||
script_files: Sequence[_JavaScript] | ||
language: str | None | ||
css_files: Sequence[_CascadingStyleSheet] | ||
sphinx_version: str | ||
sphinx_version_tuple: tuple[int, int, int, str, int] | ||
docutils_version_info: tuple[int, int, int, str, int] | ||
styles: Sequence[str] | ||
rellinks: Sequence[tuple[str, str, str, str]] | ||
builder: str | ||
parents: Sequence[_NavigationRelation] | ||
logo_url: str | ||
logo_alt: str | ||
favicon_url: str | ||
html5_doctype: Literal[True] | ||
|
||
class _PathtoCallable(Protocol): | ||
def __call__( | ||
self, otheruri: str, resource: bool = False, baseuri: str = ... | ||
) -> str: ... | ||
|
||
class _ToctreeCallable(Protocol): | ||
def __call__(self, **kwargs: Any) -> str: ... | ||
|
||
class _PageContextHTML(_GlobalContextHTML): | ||
# get_doc_context() | ||
prev: Sequence[_NavigationRelation] | ||
next: Sequence[_NavigationRelation] | ||
title: str | ||
meta: dict[str, Any] | None | ||
body: str | ||
metatags: str | ||
sourcename: str | ||
toc: str | ||
display_toc: bool | ||
page_source_suffix: str | ||
|
||
# handle_page() | ||
pagename: str | ||
current_page_name: str | ||
encoding: str | ||
pageurl: str | None | ||
pathto: _PathtoCallable | ||
hasdoc: Callable[[str], bool] | ||
toctree: _ToctreeCallable | ||
content_root: str | ||
css_tag: Callable[[_CascadingStyleSheet], str] | ||
js_tag: Callable[[_JavaScript], str] | ||
|
||
# add_sidebars() | ||
sidebars: Sequence[str] | None | ||
|
||
else: | ||
_NavigationRelation = dict | ||
_GlobalContextHTML = dict | ||
_PageContextHTML = dict |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,8 +30,7 @@ | |
from tests.test_builders.xpath_util import check_xpath | ||
|
||
if TYPE_CHECKING: | ||
from typing import Any | ||
|
||
from sphinx.builders.html._ctx import _PageContextHTML | ||
from sphinx.testing.util import SphinxTestApp | ||
|
||
|
||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. In this test If so, it seems as though the type hint of If not, it seems as though this test should change to be realistic by actually having |
||
|
||
# default for alabaster | ||
app.build(force_all=True) | ||
|
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 ofstr -> Any
" -- e.g. we add thesetheme_{...}
keys, theself.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
andextra_context
variables - meaning there is always type checking for thebase_context
. I haven't looked into the details of where this is used and what trade-offs are there.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.