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

General typing improvement #610

Merged
merged 4 commits into from Apr 29, 2024
Merged

Conversation

Sachaa-Thanasius
Copy link
Contributor

Essentially just adds a few more types to the public interface and some of the internal interface. I tried to avoid any changes to logic or configuration, though I added pyright configuration to pyproject.toml to aid these additions. That section can easily be removed.

Upon running tox, all tests seem to pass.

Copy link

codecov bot commented Apr 24, 2024

Codecov Report

Attention: Patch coverage is 85.36585% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 71.00%. Comparing base (0a4f40e) to head (c4cd0ce).

Files Patch % Lines
src/wheel/wheelfile.py 73.33% 4 Missing ⚠️
src/wheel/bdist_wheel.py 90.62% 3 Missing ⚠️
src/wheel/_setuptools_logging.py 0.00% 2 Missing ⚠️
src/wheel/macosx_libfile.py 85.71% 2 Missing ⚠️
src/wheel/cli/convert.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #610      +/-   ##
==========================================
- Coverage   71.28%   71.00%   -0.28%     
==========================================
  Files          13       13              
  Lines        1069     1083      +14     
==========================================
+ Hits          762      769       +7     
- Misses        307      314       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@agronholm
Copy link
Contributor

agronholm commented Apr 24, 2024

Essentially just adds a few more types to the public interface

There is no public interface, and the README says as much. See #262 for a discussion on that.

@Sachaa-Thanasius
Copy link
Contributor Author

Sachaa-Thanasius commented Apr 24, 2024

Fair enough, bad choice of words on my part. I used the word “public” because I’ve recently seen a few uses of bdist_wheel subclasses in setup.py files. Seems like a known way of making sure wheels are marked as abi3.

I figured improving the typing overall might make such usage a bit easier while not affecting anything the wheel package actually does.

@Sachaa-Thanasius
Copy link
Contributor Author

Even if all this provides is a few quality of life improvements for typing users who have wheel directly in their code, I hope that's still in the library's interests to have (even without an established public API). Anything I can do to fix this or make it more appealing, e.g. removing the pyright configuration?

@Sachaa-Thanasius
Copy link
Contributor Author

Sachaa-Thanasius commented Apr 28, 2024

Just following up, @agronholm:

Would you consider this PR, even if it doesn't change functionality at all? Might be unnecessary, but to make my case again:

It's more like documentation or typing aid for typing users that use wheel at runtime (even without a public interface/API). As previously mentioned, it doesn't seem uncommon to use wheel in setup.py by subclassing bdist_wheel for modification of distribution metadata, e.g. setting py_limited_api based on Python version and platform. Making it a tiny bit easier for some users to do that seems like a net benefit, but I'm not sure if you'd prefer fewer annotations to more. That's perfectly valid, but there's a mix of typed and untyped code in the package already, and I'd perhaps consider this PR as an improvement of that ratio?

Hope my reasons makes sense, and I'm more than willing to take feedback if I'm wrong.

@agronholm
Copy link
Contributor

Just following up, @agronholm:

Would you consider this PR, even if it doesn't change functionality at all? Might be unnecessary, but to make my case again:

It's more like documentation or typing aid for typing users that use wheel at runtime (even without a public interface/API). As previously mentioned, it doesn't seem uncommon to use wheel in setup.pyby subclassingbdist_wheelfor modification of distribution metadata, e.g. settingpy_limited_api` based on Python version and platform. Making it a tiny bit easier for some users to do that seems like a net benefit, but I'm not sure if you'd prefer fewer annotations to more. That's perfectly valid, but there's a mix of typed and untyped code in the package already, and I'd perhaps consider this PR as an improvement of that ratio?

Hope my reasons makes sense, and I'm more than willing to take feedback if I'm wrong.

Well, so long as we only use these annotations to sort of improve the internal documentation, and don't advertise them by adding a py.typed marker, I don't think I have a problem with it.

@Sachaa-Thanasius
Copy link
Contributor Author

Sachaa-Thanasius commented Apr 28, 2024

More than fair, thank you. If there's anything else I can do, don't hesitate to let me know.

Copy link
Contributor

@agronholm agronholm left a comment

Choose a reason for hiding this comment

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

I added some missing annotations, and commented on the pyright config.

src/wheel/bdist_wheel.py Outdated Show resolved Hide resolved
src/wheel/bdist_wheel.py Outdated Show resolved Hide resolved
src/wheel/cli/__init__.py Outdated Show resolved Hide resolved
src/wheel/cli/__init__.py Outdated Show resolved Hide resolved
src/wheel/cli/__init__.py Outdated Show resolved Hide resolved
src/wheel/cli/__init__.py Outdated Show resolved Hide resolved
src/wheel/cli/__init__.py Outdated Show resolved Hide resolved
src/wheel/cli/convert.py Outdated Show resolved Hide resolved
src/wheel/cli/convert.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
Add a few more annotations and remove pyright config.

Co-authored-by: Alex Grönholm <alex.gronholm@nextday.fi>
@agronholm agronholm merged commit 754a238 into pypa:main Apr 29, 2024
18 checks passed
@agronholm
Copy link
Contributor

Thanks.

@Sachaa-Thanasius
Copy link
Contributor Author

No problem! Thanks for considering it.

@Sachaa-Thanasius Sachaa-Thanasius deleted the typing-improvement branch May 1, 2024 02:04
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.

None yet

2 participants