Skip to content

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

Merged
merged 1 commit into from
Feb 15, 2025

Conversation

ppentchev
Copy link
Contributor

@ppentchev ppentchev commented Feb 7, 2025

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 the libtmux.test module has the from typing_extensions import Self line outside of a TYPE_CHECKING block, this led to the tests failing, since the running Python interpreter (correctly) could not find a typing_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 of typing_extensions conditional in the first place, since recent versions of Python do not really need that?" - hence the second commit that checks sys.version_info and imports Self and TypeAlias from typing instead of typing_extensions if possible.

I think that a dependency on typing_extensions should also be added to the pyproject.toml file - either unconditional as the code stands right now, or conditional on python_version < "3.11" or something. However, I have not yet used uv 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:

  • Fix an issue where tests would fail if typing-extensions was not installed.

Copy link

sourcery-ai bot commented Feb 7, 2025

Reviewer's Guide by Sourcery

This 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

Change Details Files
Adjust import for Self in the testing module to conditionally use the standard library when available.
  • Removed the unconditional import of Self from typing_extensions at the top of the file.
  • Added a conditional block to import Self from typing if Python version is 3.11 or higher, otherwise fallback to typing_extensions.
src/libtmux/test.py
Implement conditional import for TypeAlias in the server module.
  • Inserted an import of sys within the TYPE_CHECKING block.
  • Added conditional logic to import TypeAlias from typing if the Python version is 3.10 or higher, or from typing_extensions for older versions.
src/libtmux/server.py
Revise TypeAlias imports in test modules using conditional logic.
  • Added an import of sys to the TYPE_CHECKING block in test files.
  • Inserted conditional blocks to import TypeAlias from typing if available (Python 3.10+), otherwise falling back to typing_extensions, applied to both legacy and current test version files.
tests/legacy_api/test_version.py
tests/test_version.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!
  • Generate a plan of action for an issue: Comment @sourcery-ai plan on
    an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a 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

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copy link

codecov bot commented Feb 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.65%. Comparing base (d28fd30) to head (bf0d310).
Report is 1 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

@tony tony force-pushed the pp-cond-typing-extensions branch from 6d8657c to d6798ec Compare February 9, 2025 12:55
@tony
Copy link
Member

tony commented Feb 9, 2025

@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:

  1. I want to confirm we get the desired behavior from the first commit here. I will happily approve and cut a release for that.

  2. For the second commit, I propose a second PR for us to confirm it works as intended.

    I prefer avoiding reliance on typing-extensions, but it may be the case its necessary.

@tony
Copy link
Member

tony commented Feb 9, 2025

@ppentchev FYI I did a rebase of the branch against master at 6c475b2. You may need to do git pull --rebase.

@ppentchev ppentchev force-pushed the pp-cond-typing-extensions branch from d6798ec to 634bf79 Compare February 10, 2025 10:24
@shatakshiiii
Copy link

shatakshiiii commented Feb 10, 2025

We are getting a similar issue on ansible-navigator where python-3.13 test fails with ModuleNotFoundError: No module named 'typing_extensions' when it is trying to import Self from typing_extensions inside libtmux.test module

Thanks @ppentchev for coming up with the fix, +1 on this change!

@ppentchev
Copy link
Contributor Author

@ppentchev Thank you for creating this, and the detailed PR descrpition as well.

Thanks for looking into it!

Going along with what you mentioned, I propose you split the second commit into a separate PR:

1. I want to confirm we get the desired behavior from the first commit here. I will happily approve and cut a release for that.

Done, this PR now only contains the first commit. I will also adjust the PR's title in a moment.

2. For the second commit, I propose a second PR for us to confirm it works as intended.

Done, #563

   I prefer avoiding reliance on `typing-extensions`, but it may be the case its necessary.

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.

@ppentchev ppentchev changed the title Only use typing-extensions if necessary Move a typing-extensions import into a t.TYPE_CHECKING section Feb 10, 2025
@tony tony force-pushed the pp-cond-typing-extensions branch from 634bf79 to bf0d310 Compare February 15, 2025 11:55
@tony tony self-requested a review February 15, 2025 11:56
@tony
Copy link
Member

tony commented Feb 15, 2025

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.

Good point.

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.

I will consider this in a follow up PR.

Copy link
Member

@tony tony left a 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.

@tony tony merged commit f140a40 into tmux-python:master Feb 15, 2025
23 checks passed
tony added a commit that referenced this pull request Feb 15, 2025
tony added a commit that referenced this pull request Feb 15, 2025
@tony
Copy link
Member

tony commented Feb 15, 2025

@ppentchev The dependency for typing-extensions was added via #564 to the testing and lint package groups when Python is below python 3.11.

I released v0.42.1 (https://github.com/tmux-python/libtmux/releases/tag/v0.42.1, PyPI)

If you try this, is anything different?

@shatakshiiii
Copy link

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!!

@tony
Copy link
Member

tony commented Feb 16, 2025

@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)

tony added a commit that referenced this pull request Feb 17, 2025
As discussed in #562 - a second PR that contains the change that makes the use of typing-extensions conditional on the Python version.
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