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.

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

[typehints] tests: enable mypy for linkcheck builder tests. #12160

Conversation

jayaddison
Copy link
Contributor

Feature or Bugfix

  • Refactoring

Purpose

  • Improve coverage of typechecking through the codebase.

Detail

  • Enable mypy typechecking for the tests/test_builders/test_build_linkcheck.py file.

Relates

tests/test_builders/test_build_linkcheck.py Show resolved Hide resolved
tests/test_builders/test_build_linkcheck.py Show resolved Hide resolved
tests/test_builders/test_build_linkcheck.py Show resolved Hide resolved
tests/test_builders/test_build_linkcheck.py Outdated Show resolved Hide resolved
Copy link
Contributor

@danieleades danieleades left a comment

Choose a reason for hiding this comment

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

two more comments:

  1. should this wait until after use 'types-docutils' instead of 'docutils-stubs' for docutils type annotations #12012 is merged?
  2. can you modify the mypy config so that these files are not whitelisted?

@jayaddison
Copy link
Contributor Author

Thanks @danieleades for the review! I'll respond to the other questions soon; for now:

two more comments:

1. should this wait until after [use 'types-docutils' instead of 'docutils-stubs' for docutils type annotations #12012](https://github.com/sphinx-doc/sphinx/pull/12012) is merged?

Yep, I'm OK to wait for that 👍

2. can you modify the mypy config so that these files are not whitelisted?

Odd, I thought I had done that. But I will push that change, thanks!

@jayaddison
Copy link
Contributor Author

  1. can you modify the mypy config so that these files are not whitelisted?

Odd, I thought I had done that. But I will push that change, thanks!

Ah. I did do that in commit 56ef23b - but then fumbled it by accidentally including a removal of that change in 5fed4b3.

Re-applying 56ef23b ...

pyproject.toml Outdated Show resolved Hide resolved
@jayaddison
Copy link
Contributor Author

two more comments:

1. should this wait until after [use 'types-docutils' instead of 'docutils-stubs' for docutils type annotations #12012](https://github.com/sphinx-doc/sphinx/pull/12012) is merged?

With that merged, I'm going to merge mainline again into this branch (mypy linting looks good after a fresh install of the lint+test dependencies here).

2. can you modify the mypy config so that these files are not whitelisted?

Also resolved - thanks again!

@picnixz
Copy link
Member

picnixz commented Mar 25, 2024

Could I ask you some insights on #12198? because depending on the direction, we could just have a smaller diff (at the cost of the mypy plugin). Mypy plugins do not give me the possibility to annotate an un-annotated function after :(

In addition, I'm considering merging #12199 today as well (see the rationale in the PR).

@jayaddison
Copy link
Contributor Author

Could I ask you some insights on #12198? because depending on the direction, we could just have a smaller diff (at the cost of the mypy plugin). Mypy plugins do not give me the possibility to annotate an un-annotated function after :(

In addition, I'm considering merging #12199 today as well (see the rationale in the PR).

Ok, thanks @picnixz. Since this affects other branches, my preferred order to merge would be:

  1. [mypy] explicitly exclude files instead of ignoring errors by modules #12199 (adjust the config exclude list style)
  2. [typehints] tests: enable mypy for linkcheck builder tests. #12160 (this PR - enable typehinting for the linkcheck tests)
  3. [tests] linkcheck: bind each test HTTP server to a unique port per-testcase. #12126 (resolve some conflicts, and then land the port-isolation with type-hinting also)

(and oops, I missed your edit before commenting on that PR - but I could help out with attempting the copyright method refactor if that'd be useful)

@picnixz
Copy link
Member

picnixz commented Mar 25, 2024

I'll wait for the opinion of others for the mypy exclusion

@danieleades
Copy link
Contributor

there's a significant downside to this approach compared to the [[tool.mypy.overrides]] approach, which is that for the [[tool.mypy.overrides]] approach you can shrink the whitelist not just be removing files from the whitelist, but also by making the ignored errors more fine-grained (for example by changing it from ignoring all errors to only ignoring some classes of errors)

@picnixz
Copy link
Member

picnixz commented Mar 25, 2024

(for example by changing it from ignoring all errors to only ignoring some classes of errors)

For this one, I would suggest (since we are in a test-file) using a top-file comment.

Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

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

All good cheers

@chrisjsewell chrisjsewell merged commit 9467bf7 into sphinx-doc:master May 15, 2024
23 checks passed
@jayaddison jayaddison deleted the typehints/testutils-add-sphinx-typehints branch June 10, 2024 14:14
@AA-Turner AA-Turner added this to the 7.4.0 milestone Jul 13, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants