Skip to content

Fix crash due to runtime import of typing_extensions#2537

Merged
pquentin merged 1 commit intopython-trio:masterfrom
gschaffner:missing-typing_extensions-crash
Jan 18, 2023
Merged

Fix crash due to runtime import of typing_extensions#2537
pquentin merged 1 commit intopython-trio:masterfrom
gschaffner:missing-typing_extensions-crash

Conversation

@gschaffner
Copy link
Member

@gschaffner gschaffner commented Jan 18, 2023

hi! the issue fixed in #2507 was re-introduced when #2477 was merged.

typing_extensions doesn't presently need to be a runtime dependency.
@codecov
Copy link

codecov bot commented Jan 18, 2023

Codecov Report

Merging #2537 (4bb011e) into master (4286063) will increase coverage by 1.06%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2537      +/-   ##
==========================================
+ Coverage   92.44%   93.50%   +1.06%     
==========================================
  Files         118      118              
  Lines       16336    16335       -1     
  Branches     3157     3156       -1     
==========================================
+ Hits        15101    15274     +173     
+ Misses       1105      955     -150     
+ Partials      130      106      -24     
Impacted Files Coverage Δ
trio/_core/_run.py 98.92% <ø> (+0.42%) ⬆️
trio/_dtls.py 96.54% <0.00%> (+0.38%) ⬆️
trio/tests/test_subprocess.py 96.96% <0.00%> (+0.55%) ⬆️
trio/_socket.py 95.68% <0.00%> (+0.78%) ⬆️
trio/_core/tests/test_io.py 99.29% <0.00%> (+1.05%) ⬆️
trio/tests/test_socket.py 98.21% <0.00%> (+1.13%) ⬆️
trio/tests/test_threads.py 100.00% <0.00%> (+1.56%) ⬆️
trio/_highlevel_socket.py 98.19% <0.00%> (+1.80%) ⬆️
trio/tests/test_highlevel_open_tcp_stream.py 100.00% <0.00%> (+1.83%) ⬆️
... and 9 more

@pquentin
Copy link
Member

@harahu Do you think we could add a test to avoid introducing this a third time?

@pquentin pquentin merged commit 0eb0fe8 into python-trio:master Jan 18, 2023
@gschaffner gschaffner deleted the missing-typing_extensions-crash branch January 18, 2023 07:42
@harahu
Copy link
Contributor

harahu commented Jan 18, 2023

Sorry for not picking up on this!

@pquentin My suggestion would be to, rather than adding a test, I'd make sure that dynamic analysis in CI (testing, that is) is run in an environment that is as minimal as possible. That could be achieved by separating static and dynamic analysis.

To be a bit more concrete: In my understanding, this problem was camouflaged because typing-extensions is listed in test-requirements.*. typing-extensions is a test requirement because, in this project, "test" refers both to dynamic tests, but also static analysis, like running mypy.

If we break this up into a new static-analysis-requirements.* alongside a trimmed down test-requirements.*, and we make sure pytest is run with only test-requirements.* installed, we are more likely to detect problems like this.

I'm still getting to know this project, so I don't know if my understanding is correct, so please share your thoughts.

@pquentin
Copy link
Member

Yes that's right. We could have mypy-requirements.txt with contents like:

-r test-requirements.txt
typing-extensions=...

@harahu
Copy link
Contributor

harahu commented Jan 18, 2023

Alternatively, if flake8 is run in CI, there's a neat plugin that maybe would have caught this: https://pypi.org/project/flake8-requirements/

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.

3 participants