Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix validating_code_block typecheck errors #1949

Closed
wants to merge 1 commit into from
Closed

Fix validating_code_block typecheck errors #1949

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions envoy.docs.sphinx_runner/envoy/docs/sphinx_runner/ext/validating_code_block.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,7 @@ class ValidatingCodeBlock(CodeBlock):
required_arguments = CodeBlock.required_arguments
optional_arguments = CodeBlock.optional_arguments
final_argument_whitespace = CodeBlock.final_argument_whitespace
option_spec = {"type-name": directives.unchanged}
option_spec.update(CodeBlock.option_spec)
CodeBlock.option_spec.update({'type-name': directives.unchanged})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

im not so familiar with validating code block - mostly we dont use it as it makes docs build a lot slower

my concern with this change is that its setting CodeBlock.option_spec for any subsequent usage

with python its generally an anti-pattern to use any mutable vars on the class - for this reason - other code can change/redefine it and its not obvious on the class (or for any other instantiations). Mutable vars are generally defined post-instantiation - either in __init__ or by returning a mutable var from a cached @property

in this case im surprised the Sphinx code allows it, but either way i think this is changing something it shouldnt

looking at the typing errors

envoy.docs.sphinx_runner/envoy/docs/sphinx_runner/ext/validating_code_block.py:29: error: Cannot override instance variable (previously declared on base class "CodeBlock") with class variable
envoy.docs.sphinx_runner/envoy/docs/sphinx_runner/ext/validating_code_block.py:30: error: Argument 1 to "update" of "MutableMapping" has incompatible type "Dict[str, Callable[[str], Any]]"; expected "SupportsKeysAndGetItem[str, Callable[[Optional[str]], str]]"

in not sure on cause or fix tbh

in particular im trying to understand why this is flaking on some PRs and not others

the underlying issue appears to be related to this python/typeshed#11550

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... i see - its the mypy update PR that is failing here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also relevant sphinx-doc/sphinx#12067

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iiuc this PR would still fail the newer mypy

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just realizing that all prs are failing this which is strange

trying to understand what changed - it kinda shouldnt have started without some code change

either way we need to address it asap

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checking again and we dont actually pin versions (kinda by design, but perhaps we should do so for external deps)

some are set by pants/mypy setup - not sure how they can be pinned


@cached_property
def configs(self) -> Dict:
Expand Down