Skip to content
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

Improve pyright compatibility by using TypeVar's default argument #1246

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

IvanIsCoding
Copy link
Collaborator

Closes #1242

This is a pretty annoying PR but it avoids pyright from infering the types as Unknown. Although we will only keep mypy in the CI, pyright is the base for pylance which ships with VSCode. And we got VSCode issues in the past (#832), so I think it's worth supporting.

@IvanIsCoding IvanIsCoding requested a review from mtreinish July 11, 2024 03:25
@IvanIsCoding IvanIsCoding added the stable-backport-potential This PR or issue is potentially worth backporting for inclusion in a stable branch label Jul 11, 2024
@coveralls
Copy link

coveralls commented Jul 11, 2024

Pull Request Test Coverage Report for Build 12364807676

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.868%

Totals Coverage Status
Change from base Build 12364370810: 0.0%
Covered Lines: 18284
Relevant Lines: 19072

💛 - Coveralls

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one question inline about using the generic type var instead Any. But if that doesn't work I'm fine with this.

rustworkx/__init__.pyi Outdated Show resolved Hide resolved
rustworkx/rustworkx.pyi Outdated Show resolved Hide resolved
@IvanIsCoding IvanIsCoding removed the stable-backport-potential This PR or issue is potentially worth backporting for inclusion in a stable branch label Nov 16, 2024
@IvanIsCoding
Copy link
Collaborator Author

Now that we are doing a major release, I might use the suggestion from #1242 (comment) to fix it in a more proper way using TypeVar

@IvanIsCoding IvanIsCoding added this to the 0.16.0 milestone Nov 16, 2024
@IvanIsCoding IvanIsCoding changed the title Specify Any for PyGraph and PyDiGraph type annotations for pyright compatibility Improve pyright compatibility by using TypeVar's default argument Nov 16, 2024
@IvanIsCoding IvanIsCoding changed the title Improve pyright compatibility by using TypeVar's default argument Improve pyright compatibility by using TypeVar's default argument Nov 16, 2024
@IvanIsCoding IvanIsCoding force-pushed the Specify-Any-for-PyGraph/PyDiGraph branch from 5d736ac to bd6600d Compare November 16, 2024 18:58
@IvanIsCoding
Copy link
Collaborator Author

IvanIsCoding commented Nov 17, 2024

I tested with pyright, I don't want to add it to the Nox file as of now because of the Node.js aspect but it worked. Now we get:

0 errors, 0 warnings, 0 informations 

Instead of the original error. If I use the wheel from PyPI it fails with the same error message from the Issue so I am confident the fix work. It also has a much smaller diff than the original solution so that is a win

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.

Problem type checking ancestors() with pyright
3 participants