-
Notifications
You must be signed in to change notification settings - Fork 111
Move a typing-extensions import into a t.TYPE_CHECKING section #562
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_plugin
and attempted to import it, even though my tests did not need it. Since thelibtmux.test
module has thefrom typing_extensions import Self
line outside of aTYPE_CHECKING
block, this led to the tests failing, since the running Python interpreter (correctly) could not find atyping_extensions
module installed.What do you think about these two commits? To be honest, the first one would be enough: moving the import inside a
TYPE_CHECKING
block resolves that particular problem. However, I got kind of carried away and I thought "why not make the import oftyping_extensions
conditional in the first place, since recent versions of Python do not really need that?" - hence the second commit that checkssys.version_info
and importsSelf
andTypeAlias
fromtyping
instead oftyping_extensions
if possible.I think that a dependency on
typing_extensions
should also be added to thepyproject.toml
file - either unconditional as the code stands right now, or conditional onpython_version < "3.11"
or something. However, I have not yet useduv
enough 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-extensions
was not installed.