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

Enable flake8-type-checking rule #2813

Merged
merged 27 commits into from
Nov 10, 2023
Merged

Conversation

CoolCat467
Copy link
Member

This PR enables the ruff flake8-type-checking rule (TCH).
From flake8-type-checking's description, the main benefit of this would be avoiding the overhead of type hint imports at runtime.

@CoolCat467 CoolCat467 added the typing Adding static types to trio's interface label Oct 8, 2023
@codecov
Copy link

codecov bot commented Oct 8, 2023

Codecov Report

Merging #2813 (f98500d) into master (e6e18bd) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2813      +/-   ##
==========================================
- Coverage   99.18%   99.18%   -0.01%     
==========================================
  Files         115      115              
  Lines       17611    17551      -60     
  Branches     3149     3149              
==========================================
- Hits        17468    17408      -60     
  Misses         99       99              
  Partials       44       44              
Files Coverage Δ
trio/_channel.py 100.00% <ø> (ø)
trio/_core/_asyncgens.py 100.00% <ø> (ø)
trio/_core/_ki.py 100.00% <ø> (ø)
trio/_core/_multierror.py 100.00% <ø> (ø)
trio/_core/_parking_lot.py 100.00% <ø> (ø)
trio/_core/_run.py 100.00% <100.00%> (ø)
trio/_core/_tests/test_asyncgen.py 99.08% <100.00%> (-0.01%) ⬇️
trio/_core/_tests/test_guest_mode.py 100.00% <ø> (ø)
trio/_core/_tests/test_instrumentation.py 100.00% <100.00%> (ø)
trio/_core/_tests/test_io.py 100.00% <ø> (ø)
... and 38 more

@TeamSpen210
Copy link
Contributor

I would argue against this, type hints are useful to have at runtime. This will break Sphinx for instance.

@jakkdl
Copy link
Member

jakkdl commented Oct 23, 2023

I would argue against this, type hints are useful to have at runtime. This will break Sphinx for instance.

is Sphinx == ReadTheDocs here? There are (only) 4 errors, or is the signatures in the documentation broken?

Digging around a bit, it seems you can get Sphinx to run with TYPE_CHECKING = True?
https://github.com/sphinx-doc/sphinx/blob/bb74aec2b6aa1179868d83134013450c9ff9d4d6/sphinx/ext/autodoc/importer.py#L107

@CoolCat467
Copy link
Member Author

pre-commit.ci autofix

@jakkdl
Copy link
Member

jakkdl commented Oct 25, 2023

RTD build is failing, which I'm pretty sure it's not on master, so something is being messed up somewhere.

@CoolCat467
Copy link
Member Author

RTD build is failing, which I'm pretty sure it's not on master, so something is being messed up somewhere.

Errors I'm seeing in the most recent run RTD had:

trio/__init__.py:docstring of trio.run_process:1: WARNING: py:class reference target not found: StrOrBytesPath
trio/__init__.py:docstring of trio.run_process:1: WARNING: py:class reference target not found: StrOrBytesPath
trio/lowlevel.py:docstring of trio.lowlevel.Task.iter_await_frames:1: WARNING: py:class reference target not found: FrameType
trio/lowlevel.py:docstring of trio.lowlevel.start_guest_run:1: WARNING: py:class reference target not found: Outcome

Not completely sure what this means, something about something missing from docstrings? I'm sure if I looked into it deeper I could figure it out but if anyone has more information I would be glad for the advise. Otherwise I'll do some research later and we can get this taken care of.

@jakkdl
Copy link
Member

jakkdl commented Oct 26, 2023

similar problems required aliases or similar in docs/source/conf.py, or shuffling around imports. You can run the build process locally with make html standing in the docs/ directory, which should make it much quicker to try to playing around with the code and figure out what is erroring where and why.
You could e.g. git blame the lines in it to dig up the PRs that added them, but I tried to add decent comments since Sphinx is kind of magic to me.

@TeamSpen210
Copy link
Contributor

Sphinx isn't able to find those names when it's trying to evaluate the annotations - possibly because those functions were imported into a different class or something. Probably need to move the imports out of TYPE_CHECKING. It could also be caused by us importing trio in conf.py to get __version__ - maybe change that to read the file itself, so the module isn't loaded?

@jakkdl
Copy link
Member

jakkdl commented Oct 27, 2023

Probably need to move the imports out of TYPE_CHECKING.

this shouldn't be the case if 4764ba2 is working as expected.

@CoolCat467
Copy link
Member Author

google/pytype#1066 seems to be having a similar issue with StrOrBytesPath

@jakkdl
Copy link
Member

jakkdl commented Nov 4, 2023

I kind of expected the RTD build to fail, but apparently it runs on 3.9+, whoop whoop.

google/pytype#1066 seems to be having a similar issue with StrOrBytesPath

I didn't manage to figure out any way of getting StrOrBytesPath working, but it does not seem related to the above afaict.

@CoolCat467
Copy link
Member Author

I kind of expected the RTD build to fail, but apparently it runs on 3.9+, whoop whoop.

google/pytype#1066 seems to be having a similar issue with StrOrBytesPath

I didn't manage to figure out any way of getting StrOrBytesPath working, but it does not seem related to the above afaict.

I thought I would mention it, one of the only internet search results with anything potentially related. Maybe I was just looking in the wrong places, but it seems like sphinx doesn't have very much information about fixing errors.

Thank you so much for helping with this, I was definitely struggling a bit!

@TeamSpen210
Copy link
Contributor

For StrOrBytesPath, try adding it to autodoc_type_aliases, or we could use autodoc_process_signature() to expand it. That function gets passed the entire function signature as a string, so you can edit it however you like.

@jakkdl
Copy link
Member

jakkdl commented Nov 5, 2023

For StrOrBytesPath, try adding it to autodoc_type_aliases, or we could use autodoc_process_signature() to expand it. That function gets passed the entire function signature as a string, so you can edit it however you like.

I tried autodoc_type_aliases without any success

@jakkdl jakkdl merged commit b7d2054 into python-trio:master Nov 10, 2023
27 checks passed
@jakkdl
Copy link
Member

jakkdl commented Nov 10, 2023

teamwork! 🚀

@CoolCat467 CoolCat467 deleted the ruff-enable-TCH branch November 10, 2023 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typing Adding static types to trio's interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants