Move a typing-extensions import into a t.TYPE_CHECKING section#562
Move a typing-extensions import into a t.TYPE_CHECKING section#562tony merged 1 commit intotmux-python:masterfrom
Conversation
Reviewer's Guide by SourceryThis pull request refactors the import logic for typing-related utilities by conditionally importing from the standard library when available (based on the Python version) and falling back to typing_extensions only when necessary. The main changes involve moving imports into TYPE_CHECKING blocks and adding version checks using sys.version_info to ensure compatibility with different Python versions. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @ppentchev - I've reviewed your changes - here's some feedback:
Overall Comments:
- The version checks and conditional imports for Self/TypeAlias repeat across modules; consider centralizing this logic into a helper to reduce duplication.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #562 +/- ##
==========================================
- Coverage 88.67% 88.65% -0.03%
==========================================
Files 36 36
Lines 3922 3921 -1
Branches 362 362
==========================================
- Hits 3478 3476 -2
Misses 307 307
- Partials 137 138 +1 ☔ View full report in Codecov by Sentry. |
6d8657c to
d6798ec
Compare
|
@ppentchev Thank you for creating this, and the detailed PR descrpition as well. Going along with what you mentioned, I propose you split the second commit into a separate PR:
|
|
@ppentchev FYI I did a rebase of the branch against master at 6c475b2. You may need to do |
d6798ec to
634bf79
Compare
|
We are getting a similar issue on ansible-navigator where Thanks @ppentchev for coming up with the fix, +1 on this change! |
Thanks for looking into it!
Done, this PR now only contains the first commit. I will also adjust the PR's title in a moment.
Done, #563
Well, hm. Actually, if it is only referenced in TYPE_CHECKING blocks anyway, the argument could be made that mypy will bring it in - and it will never be imported in any other case. Still, I personally would still add it as a testing dependency, but it's really up to you. |
634bf79 to
bf0d310
Compare
Good point.
I will consider this in a follow up PR. |
There was a problem hiding this comment.
LGTM
I will investigate the typing-extensions dependency next.
|
@ppentchev The dependency for I released v0.42.1 (https://github.com/tmux-python/libtmux/releases/tag/v0.42.1, PyPI) If you try this, is anything different? |
|
Thank you for merging this PR and rolling out a new release. I can confirm that the above mentioned failures are fixed and related tests are passing in ansible-navigator now with the latest version of libtmux. TY @tony @ppentchev!! |
|
@shatakshiiii Superb, happy this did the trick, in re: ansible/ansible-navigator#1918! @ppentchev I will keep an eye on this to confirm this resolved issue downstream for you, just in case! (I assume it'll do) |
As discussed in #562 - a second PR that contains the change that makes the use of typing-extensions conditional on the Python version.
Hi,
Thanks for your continued work on libtmux!
Now here's a weird one. in a Debian package that I maintain, I wrote some tests that use libtmux and that are run by pytest. In the definition of the test environment for the Debian package, I listed libtmux and pytest as packages to be installed; neither of them brings in the typing-extensions library as a dependency, at least on recent versions of Python.
Now, when those tests were run, pytest detected the presence of
libtmux.pytest_pluginand attempted to import it, even though my tests did not need it. Since thelibtmux.testmodule has thefrom typing_extensions import Selfline outside of aTYPE_CHECKINGblock, this led to the tests failing, since the running Python interpreter (correctly) could not find atyping_extensionsmodule installed.What do you think about these two commits? To be honest, the first one would be enough: moving the import inside a
TYPE_CHECKINGblock resolves that particular problem. However, I got kind of carried away and I thought "why not make the import oftyping_extensionsconditional in the first place, since recent versions of Python do not really need that?" - hence the second commit that checkssys.version_infoand importsSelfandTypeAliasfromtypinginstead oftyping_extensionsif possible.I think that a dependency on
typing_extensionsshould also be added to thepyproject.tomlfile - either unconditional as the code stands right now, or conditional onpython_version < "3.11"or something. However, I have not yet useduvenough for developing modules (I am using it heavily to create test environments, and I am very happy with it), so I thought I'd leave that to people who are more familiar with that workflow.Let me know if you think the first patch will be enough, and I will create a separate branch for it.
Thanks again, and keep up the great work!
Summary by Sourcery
Bug Fixes:
typing-extensionswas not installed.