-
Notifications
You must be signed in to change notification settings - Fork 109
Move a typing-extensions import into a t.TYPE_CHECKING section #562
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
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.