bdist_wheel typing improvement#4383
Conversation
|
Yeah, I didn't really focus on the type annotations because I feel that the whole things needs a rewrite, preferably on top of the |
dd1637c to
c44fb1e
Compare
c44fb1e to
aa867ba
Compare
setuptools/command/bdist_wheel.py
Outdated
| self.group = None | ||
| self.universal: bool = False | ||
| self.compression: str | int = "deflated" | ||
| self.compression: int = ZIP_DEFLATED |
There was a problem hiding this comment.
Umm... that seems to be a runtime "lie".
We know for sure that self.compression will have an str value temporarily assigned to it (if the --compression flag is given in sys.argv).
If a developer believes this info, it may generate problems later?
There was a problem hiding this comment.
By developer, do you mean someone working with bdist_wheel code?
I feel that this change is ill advised. If anything, we should use a separate member for the "translated" compression option.
There was a problem hiding this comment.
By developer, do you mean someone working with bdist_wheel code?
Yes.
But I agree with you a better change would be separating into 2 different properties, or some arrangement similar to that.
There was a problem hiding this comment.
It looks like it intended to allow using a str as the initial value for human readability, that gets converted into its equivalent compression type int.
The problem is that you can set compression manually after finalize_options but before run, which would result in an error if you use a str. Hence compression cannot be a str* unless you expect the code using to to reconvert/recheck everytime.
The idea of allowing a string for human readability is kinda obsoleted by zipfile constants (ZIP_DEFLATED, ZIP_STORED, etc.).
Allowing a compression parameter as a string (like in __init__ ) is fine, because you can convert it in that method before it ever reached the compression attribute. So I kept that feature in.
* Whilst it can't be a str and have sane & safe typing, what it could be is a Descriptor (or property with setter):
- setter that accepts both an int or str and does the conversion imediatly
- getter that returns the int value
This would also move that logic out of finalize_options.
There was a problem hiding this comment.
This would also move that logic out of
finalize_options.
I think that is fine... it is only used once and it is a relatively simple computation, so there is no need for caching... We can just defer until we actually need it.
Maybe something like the following:
diff --git i/setuptools/command/bdist_wheel.py w/setuptools/command/bdist_wheel.py
index 7bfb3fb02..97aed9428 100644
--- i/setuptools/command/bdist_wheel.py
+++ w/setuptools/command/bdist_wheel.py
@@ -248,7 +248,7 @@ class bdist_wheel(Command):
self.owner = None
self.group = None
self.universal: bool = False
- self.compression: int = ZIP_DEFLATED
+ self.compression: str | int = "deflated"
self.python_tag: str = python_tag()
self.build_number: str | None = None
self.py_limited_api: str | Literal[False] = False
@@ -265,18 +265,6 @@ class bdist_wheel(Command):
self.data_dir = self.wheel_dist_name + ".data"
self.plat_name_supplied = bool(self.plat_name)
- # Handle compression not being an int or a supported value
- if not (
- isinstance(self.compression, int)
- and self.compression in self.supported_compressions.values()
- ):
- try:
- self.compression = self.supported_compressions[str(self.compression)]
- except KeyError:
- raise ValueError(
- f"Unsupported compression: {self.compression}"
- ) from None
-
need_options = ("dist_dir", "plat_name", "skip_build")
self.set_undefined_options("bdist", *zip(need_options, need_options))
@@ -304,6 +292,19 @@ class bdist_wheel(Command):
if self.build_number is not None and not self.build_number[:1].isdigit():
raise ValueError("Build tag (build-number) must start with a digit.")
+ def _zip_compression(self) -> int:
+ # Handle compression not being an int or a supported value
+ if (
+ isinstance(self.compression, int)
+ and self.compression in self.supported_compressions.values()
+ ):
+ return self.compression
+
+ try:
+ return self.supported_compressions[str(self.compression)]
+ except KeyError:
+ raise ValueError(f"Unsupported compression: {self.compression}") from None
+
@property
def wheel_dist_name(self) -> str:
"""Return distribution full name with - replaced with _"""
@@ -443,7 +444,7 @@ class bdist_wheel(Command):
os.makedirs(self.dist_dir)
wheel_path = os.path.join(self.dist_dir, archive_basename + ".whl")
- with WheelFile(wheel_path, "w", self.compression) as wf:
+ with WheelFile(wheel_path, "w", self._zip_compression()) as wf:
wf.write_files(archive_root)
# Add to 'Distribution.dist_files' so that the "upload" command worksThere was a problem hiding this comment.
That works too. As you said, one difference with the Descriptor/property approach I was suggesting is caching the value.
Another difference is that the error will happen on usage rather than when setting an invalid value.
You still have to remember to use _zip_compression, and not compression directly
setuptools/command/bdist_wheel.py
Outdated
| and self.compression in self.supported_compressions.values() | ||
| ): | ||
| try: | ||
| self.compression = self.supported_compressions[str(self.compression)] |
There was a problem hiding this comment.
I can see how the str(...) cast is deceiving the typechecker to make it happy: so it believes that self.compression is an integer in runtime... but we know it can actually be a string...
There was a problem hiding this comment.
I think the idea of using a Descriptor or property would clean this up https://github.com/pypa/setuptools/pull/4383/files#r1646513416
setuptools/command/bdist_wheel.py
Outdated
| try: | ||
| if isinstance(self.compression, str): | ||
| return self.supported_compressions[self.compression] | ||
| except KeyError: | ||
| pass |
There was a problem hiding this comment.
This could be done w/o a try-except.
| try: | |
| if isinstance(self.compression, str): | |
| return self.supported_compressions[self.compression] | |
| except KeyError: | |
| pass | |
| if isinstance(self.compression, str): | |
| compression = self.supported_compressions.get(self.compression) | |
| if compression is not None: | |
| return compression |
And just for fun, using the walrus operator (Assignment Expressions, which I don't like)
| try: | |
| if isinstance(self.compression, str): | |
| return self.supported_compressions[self.compression] | |
| except KeyError: | |
| pass | |
| if isinstance(self.compression, str) and ( | |
| compression := self.supported_compressions.get(self.compression) | |
| ): | |
| return compression |
…wheel-typing-improvement
Co-authored-by: Avasam <samuel.06@hotmail.com>
|
Thank you very much! |
Summary of changes
A couple things I noticed from #4369 CC @agronholm
Pull Request Checklist
newsfragments/.(See documentation for details)