Conversation
daniel-sanche
added
the
do not merge
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the cryptography module to simplify RSA signer and verifier implementations by replacing dynamic wrapper classes with direct aliasing based on library availability. It also promotes the rsa library to a core dependency, removes associated deprecation warnings, and streamlines the CI configuration. The review feedback identifies several critical issues: a potential side-effect in noxfile.py caused by in-place list modification, a regression in the test runner's ability to accept positional arguments, and a packaging bug in setup.py resulting in a nested dependency list. Additionally, improvements were suggested for type-hinting specificity and minor documentation grammar.
There was a problem hiding this comment.
Removing .copy() causes ALL_PYTHON to be a reference to the UNIT_TEST_PYTHON_VERSIONS list. The subsequent extend call on line 46 will modify the global UNIT_TEST_PYTHON_VERSIONS list in place, which can lead to unexpected side effects in other sessions that rely on the original list.
| ALL_PYTHON = UNIT_TEST_PYTHON_VERSIONS | |
| ALL_PYTHON = UNIT_TEST_PYTHON_VERSIONS.copy() |
| "tests", | ||
| "tests_async", | ||
| *session.posargs, | ||
| ) |
There was a problem hiding this comment.
The removal of *session.posargs prevents users from passing additional arguments to pytest (e.g., to run specific tests or use specific flags). This is a regression in developer experience.
| ) | |
| *session.posargs, | |
| ) |
| enterprise_cert_extra_require = ["cryptography", "pyopenssl"] | ||
|
|
||
| pyopenssl_extra_require = ["pyopenssl>=20.0.0"] | ||
| pyopenssl_extra_require = ["pyopenssl>=20.0.0", cryptography_base_require] |
There was a problem hiding this comment.
This creates a nested list: ["pyopenssl>=20.0.0", ["cryptography >= 38.0.3"]]. Setuptools expects a flat list of requirement strings. You should unpack cryptography_base_require using the * operator.
| pyopenssl_extra_require = ["pyopenssl>=20.0.0", cryptography_base_require] | |
| pyopenssl_extra_require = ["pyopenssl>=20.0.0", *cryptography_base_require] |
| es = None # type: ignore | ||
| es256 = None # type: ignore |
There was a problem hiding this comment.
According to the general rules, when assigning None to a variable in an except ImportError block to handle optional dependencies, type: ignore[assignment] should be used instead of a plain type: ignore to be more specific for static analysis tools.
| es = None # type: ignore | |
| es256 = None # type: ignore | |
| es = None # type: ignore[assignment] | |
| es256 = None # type: ignore[assignment] |
References
- When handling an optional dependency by assigning None in an except ImportError block, type: ignore[assignment] is necessary on the assignment to prevent a mypy error, as using typing.TYPE_CHECKING alone is insufficient.
| @classmethod | ||
| def from_string(cls, public_key: Union[str, bytes]) -> "EsVerifier": | ||
| """Construct a Verifier instance from a public key or public | ||
| """Construct an Verifier instance from a public key or public |
There was a problem hiding this comment.
Grammar error: 'Verifier' starts with a consonant sound, so 'a' should be used instead of 'an'.
| """Construct an Verifier instance from a public key or public | |
| """Construct a Verifier instance from a public key or public |
| RSASigner = _python_rsa.RSASigner # type: ignore | ||
| RSAVerifier = _python_rsa.RSAVerifier # type: ignore |
There was a problem hiding this comment.
Similar to the rule for optional dependencies, using a more specific type: ignore[assignment] is preferred over a broad type: ignore when aliasing classes from different implementations to the same name, as static analysis tools like mypy will flag the type mismatch on reassignment.
| RSASigner = _python_rsa.RSASigner # type: ignore | |
| RSAVerifier = _python_rsa.RSAVerifier # type: ignore | |
| RSASigner = _python_rsa.RSASigner # type: ignore[assignment] | |
| RSAVerifier = _python_rsa.RSAVerifier # type: ignore[assignment] |
References
- When handling an optional dependency by assigning None in an except ImportError block, type: ignore[assignment] is necessary on the assignment to prevent a mypy error, as using typing.TYPE_CHECKING alone is insufficient.
| # TODO(https://github.com/googleapis/google-auth-library-python/issues/1738): Add bounds for pyopenssl dependency. | ||
| enterprise_cert_extra_require = ["pyopenssl"] | ||
| # TODO(https://github.com/googleapis/google-auth-library-python/issues/1738): Add bounds for cryptography and pyopenssl dependencies. | ||
| enterprise_cert_extra_require = ["cryptography", "pyopenssl"] |
There was a problem hiding this comment.
To satisfy the TODO regarding adding bounds for cryptography, and for consistency with other extra requirements in this file, consider using the cryptography_base_require list.
1 participant
Test revert commits that removed rsa, to see if the build passes
This will not be released, but may be useful as a temporary branch