Skip to content

Conversation

@2bndy5
Copy link
Collaborator

@2bndy5 2bndy5 commented Jul 16, 2024

Replace tools black and pylint with ruff

  • removes # pylint: disable comments
  • uses ruff # noqa coments to disable some checks in certain contexts

Note

The ruff check command respects the project's metadata python-requires. Whereas pylint would only focus on the version of python that was used to run it.

See ruff docs for more detail.

Fix/abate typing

As per usual updates to mypy and sphinx, some typing warnings had to be suppressed in monkeypatching code.

The only change in sphinx that might need attention is the change in sphinx.directives use of sphinx.util.nodes.nested_parse_with_titles() which we are monkey patching for sphinx<6.2.0 only. See sphinx-doc/sphinx#12492 for change in v7.4.0 and the fix that the monkey patch applies in sphinx-doc/sphinx#11147.

2bndy5 added 4 commits July 15, 2024 19:16
ran `ruff check` and `ruff format`
See sphinx-doc/sphinx#12492

The mypy warning only applies to `sphinx>=v7.4`, but the code is only executed for `sphinx<v6.2.0`.
@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 16, 2024

The docs' build error seems unrelated to these changes.

/opt/hostedtoolcache/Python/3.12.4/x64/lib/python3.12/site-packages/sphinx_immaterial/custom_admonitions.py:docstring of sphinx_immaterial.custom_admonitions.CustomAdmonitionConfig.title:1: WARNING: py:obj reference target not found: typing.Annotated[str | None, FieldInfo(annotation=NoneType, required=True, validate_default=True)]
/opt/hostedtoolcache/Python/3.12.4/x64/lib/python3.12/site-packages/sphinx_immaterial/custom_admonitions.py:docstring of sphinx_immaterial.custom_admonitions.CustomAdmonitionConfig.classes:1: WARNING: py:obj reference target not found: typing.List[~typing.Annotated[str, AfterValidator(func=<function make_id at 0x7fd2a7b5cae0>)]]

I suspect there's something different with Sphinx' writer.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 16, 2024

I found the change in sphinx-doc/sphinx#11785. I'm not sure if there's something we need to do to compensate.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 17, 2024

I found the culprit. So, sphinx_immaterial.apidoc.python.attribute_style) monkey-patches the handle_signature() function of sphinx.domains.python.PyAttribute (and PyProperty) to convert the signature's type into xrefs using a monkey-patched sphinx.domains.python._parse_annotation().

I'm not yet sure where to go from here. I'd prefer to skip displaying the typing.Annotated metadata all together because they are implementation details. If we do decide to start supporting Annotated metadata, then we would need to way to allow ignoring unresolved xref warnings in the signature's type.

@jbms
Copy link
Owner

jbms commented Jul 17, 2024

I agree that the Annotated type annotations are not helpful to display in our documentation.

I don't think this problem necessarily has much to do with our monkey patching, which affects how Python attributes are displayed --- those same Annotated types could still occur elsewhere that would already be displayed using _parse_annotation.

One trick I've used is to set some env var or other indication in conf.py and then condition the type annotations depending on whether docs are being generated, similar to conditioning on if TYPE_CHECKING. In particular we would arrange things so that it is just the bare unannotated types when generating our own docs. However, that isn't ideal because then it means the validation would also be skipped when generating our own docs.

Is there a way to do validation with pydantic that does not rely on Annotated types?

Alternatively we could add some options to apidoc/python/type_annotation_transforms.py to exclude Annotations that match certain user-defined patterns.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 17, 2024

Is there a way to do validation with pydantic that does not rely on Annotated types?

I think so. I haven't looked at their docs in a while. But I think this fix would only be specific to our docs. Like you said, this might already be affecting end-user doc projects.

Alternatively we could add some options to apidoc/python/type_annotation_transforms.py to exclude Annotations that match certain user-defined patterns.

This is the first place I started looking when trying to find the culprit. I would favor something similar to python_type_aliases.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 18, 2024

I don't think this problem necessarily has much to do with our monkey patching

You're right. These warnings only surface because our docs use nitpicky mode

nitpicky = True

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 18, 2024

I can add to our conf.py's nitpicky_ignore list, but the type using a pydantic.AfterValidator(docutils.nodes.make_id) cannot be ignored because the __str__() for docutils.nodes.make_id() shows the function's memory offset (which is different on every build).

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 18, 2024

I opted for "avoid doc-ing Annotated attrs" approach because our docs use --nitpicky --fail-on-warnings options.

This is being tracked upstream in sphinx-doc/sphinx#12601. Although, I must say it was difficult to determine if it is specific to our theme or not because of the various monkey-patching and my unfamiliarity with sphinx autodoc/domains API.

@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2024

Codecov Report

Attention: Patch coverage is 71.62162% with 21 lines in your changes missing coverage. Please review.

Project coverage is 63.91%. Comparing base (4cc8b81) to head (a1b0bfd).
Report is 29 commits behind head on main.

Files with missing lines Patch % Lines
sphinx_immaterial/apidoc/python/apigen.py 25.00% 6 Missing ⚠️
sphinx_immaterial/apidoc/cpp/api_parser.py 37.50% 5 Missing ⚠️
sphinx_immaterial/sphinx_utils.py 50.00% 2 Missing ⚠️
sphinx_immaterial/apidoc/apigen_utils.py 0.00% 1 Missing ⚠️
sphinx_immaterial/apidoc/cpp/symbol_ids.py 0.00% 1 Missing ⚠️
sphinx_immaterial/apidoc/cpp/synopses.py 0.00% 1 Missing ⚠️
sphinx_immaterial/apidoc/format_signatures.py 0.00% 1 Missing ⚠️
...nx_immaterial/apidoc/object_description_options.py 0.00% 1 Missing ⚠️
sphinx_immaterial/apidoc/python/domain_fixes.py 0.00% 1 Missing ⚠️
sphinx_immaterial/apidoc/python/object_ids.py 0.00% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #370   +/-   ##
=======================================
  Coverage   63.91%   63.91%           
=======================================
  Files          67       67           
  Lines        8070     8068    -2     
=======================================
- Hits         5158     5157    -1     
+ Misses       2912     2911    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 18, 2024

@jbms could you install the CodeCov app on this repo? The CodeCov site is unable to list coverage differences between head and base branches.

@2bndy5 2bndy5 merged commit 7796775 into main Jul 18, 2024
@2bndy5 2bndy5 deleted the update-linters branch July 18, 2024 03:05
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.

4 participants