bdist_wheel typing improvement

[Avasam]

Summary of changes

A couple things I noticed from #4369 CC @agronholm

Pull Request Checklist

• Changes have tests (existing tests + mypy checks newly completed methods)
• News fragment added in newsfragments/.
  (See documentation for details)

[agronholm]

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 packaging APIs I'm currently working on.

[abravalheri]

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?

[agronholm]

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.

[abravalheri]

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.

[Avasam]

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.

[abravalheri]

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 works

[Avasam]

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

[abravalheri]

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

[Avasam]

I think the idea of using a Descriptor or property would clean this up https://github.com/pypa/setuptools/pull/4383/files#r1646513416

[Avasam]

This could be done w/o a try-except.

Suggested change

	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)

Suggested change

	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

[abravalheri]

Thank you very much!