Skip to content

Conversation

@danieleades
Copy link
Contributor

No description provided.

@github-actions

This comment has been minimized.

@danieleades danieleades force-pushed the docutils.nodes.Element branch from ec43ca6 to 83b6563 Compare March 8, 2024 07:35
@github-actions

This comment has been minimized.

@danieleades
Copy link
Contributor Author

could really use your eyes on this one @picnixz!

@picnixz
Copy link
Member

picnixz commented Mar 8, 2024

For this one I'll need my computer because I don't think a review from my phone would be precise enough. Thus, I'll do it next week.

@danieleades danieleades force-pushed the docutils.nodes.Element branch 2 times, most recently from b75aafd to 246f3de Compare March 8, 2024 11:49
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@danieleades danieleades force-pushed the docutils.nodes.Element branch from b17aa64 to a40d252 Compare March 8, 2024 15:57
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

def replace(self, old: Node, new: Node | Sequence[Node]) -> None: ...
def replace_self(self, new: Node | Sequence[Node]) -> None: ...
def first_child_matching_class(
self, childclass: type[Node] | tuple[type[Node], ...], start: int = 0, end: int = sys.maxsize
Copy link
Member

Choose a reason for hiding this comment

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

Check if childclass also allow unions and union types

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@danieleades danieleades force-pushed the docutils.nodes.Element branch from efce590 to 087e590 Compare March 10, 2024 12:03
@github-actions

This comment has been minimized.

@JelleZijlstra
Copy link
Member

Could you fix the merge conflict?

@danieleades danieleades force-pushed the docutils.nodes.Element branch from 087e590 to aaa1414 Compare March 12, 2024 07:07
@danieleades danieleades force-pushed the docutils.nodes.Element branch from aaa1414 to 0a83560 Compare March 13, 2024 14:20
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@danieleades
Copy link
Contributor Author

+ sphinx/environment/collectors/metadata.py:32:56: error: Argument 1 to "first_child_not_matching_class" of "Element" has incompatible type "type[PreBibliographic]"; expected "Union[type[Node], tuple[type[Node], ...]]"  [arg-type]

@JelleZijlstra this is an interesting one. It seems not all Nodes subclass Node...

PreBibliographic has no base class

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

See below for some notes. Also, why did you remove child_text_separator from some of the classes?

def index(self, item: Node, start: int = 0, stop: int = sys.maxsize) -> int: ...
def previous_sibling(self) -> Node | None: ...
def __getattr__(self, name: str, /) -> Incomplete: ...
def is_not_default(self, key: str) -> Literal[0, 1]: ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder whether it's better to just return bool here, as this is clearly how it's intended to be used. Using Literal[0, 1] will prevent the return value from being used as a bool, which works at runtime.

@danieleades
Copy link
Contributor Author

why did you remove child_text_separator from some of the classes?

because this is inherited from the Element base class and doesn't need to be redefined

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

sphinx (https://github.com/sphinx-doc/sphinx)
+ sphinx/addnodes.py: note: In member "preserve_original_messages" of class "toctree":
+ sphinx/addnodes.py:91:22: error: Need type annotation for "rawentries"  [var-annotated]
- sphinx/util/nodes.py: note: In function "_copy_except__document":
- sphinx/util/nodes.py:658:5: error: "Element" has no attribute "tagname"  [attr-defined]
- sphinx/util/nodes.py:660:5: error: "Element" has no attribute "attributes"  [attr-defined]
+ sphinx/directives/__init__.py: note: In member "run" of class "ObjectDescription":
+ sphinx/directives/__init__.py:299:28: error: Need type annotation for "node_id"  [var-annotated]
+ sphinx/domains/std/__init__.py: note: In member "add_target_and_index" of class "Cmdoption":
+ sphinx/domains/std/__init__.py:209:9: error: Need type annotation for "optname"  [var-annotated]
+ sphinx/domains/std/__init__.py:231:9: error: Need type annotation for "option"  [var-annotated]
+ sphinx/transforms/i18n.py: note: In member "update_title_mapping" of class "_NodeUpdater":
+ sphinx/transforms/i18n.py:140:25: error: Need type annotation for "names"  [var-annotated]
+ sphinx/transforms/i18n.py: note: In member "apply" of class "AddTranslationClasses":
+ sphinx/transforms/i18n.py:590:59: error: Argument 1 to "append" of "list" has incompatible type "str"; expected Never  [arg-type]
+ sphinx/transforms/i18n.py:593:59: error: Argument 1 to "append" of "list" has incompatible type "str"; expected Never  [arg-type]
+ sphinx/writers/latex.py: note: In member "visit_inline" of class "LaTeXTranslator":
+ sphinx/writers/latex.py:2124:9: error: Need type annotation for "classes" (hint: "classes: List[<type>] = ...")  [var-annotated]
+ sphinx/writers/latex.py: note: In member "visit_container" of class "LaTeXTranslator":
+ sphinx/writers/latex.py:2156:9: error: Need type annotation for "classes" (hint: "classes: List[<type>] = ...")  [var-annotated]
+ sphinx/writers/latex.py: note: In member "depart_container" of class "LaTeXTranslator":
+ sphinx/writers/latex.py:2161:9: error: Need type annotation for "classes" (hint: "classes: List[<type>] = ...")  [var-annotated]
- sphinx/environment/collectors/metadata.py:37: error: Unused "type: ignore" comment  [unused-ignore]
+ sphinx/environment/collectors/metadata.py: note: In member "process_doc" of class "MetadataCollector":
+ sphinx/environment/collectors/metadata.py:32:56: error: Argument 1 to "first_child_not_matching_class" of "Element" has incompatible type "type[PreBibliographic]"; expected "Union[type[Node], tuple[type[Node], ...]]"  [arg-type]

@srittau srittau merged commit fa9dc6d into python:main Mar 16, 2024
@danieleades danieleades deleted the docutils.nodes.Element branch March 16, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants