Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Still requires adjustments but marking so you know someone is willing to work on it |
There was a problem hiding this comment.
Code Review
This pull request adds type hints to various modules within the google-auth library and introduces a new internal helper file. The review identifies critical issues where stub syntax was incorrectly used in Python implementation files, which would cause runtime failures. Furthermore, the feedback points out several missing imports for type annotations, numerous redundant duplicate imports, and the need for specific type ignore markers when handling optional dependencies.
| class _RequestMethodsBase: ... | ||
|
|
||
| class TimeoutGuard: | ||
| remaining_timeout: Any | ||
|
|
||
| def __init__( | ||
| self, | ||
| timeout: Any, | ||
| timeout_error_type: type[Exception] = requests.exceptions.Timeout, | ||
| ) -> None: ... | ||
| def __enter__(self) -> Self: ... | ||
| def __exit__( | ||
| self, | ||
| exc_type: type[BaseException] | None, | ||
| exc_value: BaseException | None, | ||
| traceback: types.TracebackType | None, | ||
| ) -> None: ... | ||
|
|
||
| class _MutualTlsAdapter(HTTPAdapter): | ||
| def __init__(self, cert: bytes, key: bytes) -> None: ... | ||
| def init_poolmanager(self, *args: Any, **kwargs: Any) -> None: ... | ||
| def proxy_manager_for(self, *args: Any, **kwargs: Any): ... | ||
|
|
||
| class _MutualTlsOffloadAdapter(HTTPAdapter): | ||
| signer: Any | ||
|
|
||
| def __init__(self, enterprise_cert_file_path: str) -> None: ... | ||
| def init_poolmanager(self, *args: Any, **kwargs: Any) -> None: ... | ||
| def proxy_manager_for(self, *args: Any, **kwargs: Any): ... | ||
|
|
There was a problem hiding this comment.
These class definitions use stub syntax (...) in a .py file. This will overwrite any existing implementations with empty classes and methods, breaking the library's functionality at runtime. Additionally, requests and types are used in these definitions but are not imported. Type definitions should be added to the actual implementation or provided in a separate .pyi stub file.
| @type_check_only | ||
| class _BaseAuthorizedSession(metaclass=abc.ABCMeta): | ||
| credentials: Any | ||
|
|
||
| def __init__(self, credentials: Any) -> None: ... | ||
| @abc.abstractmethod | ||
| def request( | ||
| self, | ||
| method: str, | ||
| url: str, | ||
| data: Any = None, | ||
| headers: Any = None, | ||
| max_allowed_time: Any = None, | ||
| timeout: int = ..., | ||
| **kwargs: Any, | ||
| ): ... | ||
| @abc.abstractmethod | ||
| def close(self) -> None: ... No newline at end of file |
There was a problem hiding this comment.
This file contains several issues that will cause runtime errors:
- abc and abc.ABCMeta are used but not imported.
- @type_check_only is used but not imported or defined. This decorator is typically used in stub files (.pyi), not implementation files (.py).
- The use of ... and ellipsis in default values is also characteristic of stub files.
If this file is intended to be a runtime helper, it needs proper imports and implementation. If it's intended to be a stub, it should be renamed to _helpers.pyi.
| from collections import OrderedDict | ||
| from typing import TypeVar | ||
|
|
||
| _Key = TypeVar("_Key", bound=Hashable) |
There was a problem hiding this comment.
The type hint Hashable is used but not imported. It should be imported from collections.abc.
| _Key = TypeVar("_Key", bound=Hashable) | |
| _Key = TypeVar("_Key", bound=collections.abc.Hashable) |
| def load_credentials_from_file( | ||
| filename, scopes=None, default_scopes=None, quota_project_id=None, request=None | ||
| ): | ||
| filename: str, scopes: collections.abc.Sequence[str] | None=None, default_scopes: collections.abc.Sequence[str] | None=None, quota_project_id: str | None=None, request: Request | None=None |
There was a problem hiding this comment.
The type hint Request is used but not imported in this file. It should be imported from google.auth.transport.
| from google.auth import _helpers | ||
| from google.auth import exceptions | ||
| from google.auth._credentials_base import _BaseCredentials | ||
| from google.auth.transport import Request as _TransportRequest, Request as _TransportRequest, Request as _TransportRequest, Request as _TransportRequest, Request as _TransportRequest, Request as _TransportRequest |
There was a problem hiding this comment.
Duplicate import of Request as _TransportRequest. Remove duplicate lines of code to keep the codebase clean and avoid redundancy.
References
- Remove duplicate lines of code, especially duplicate assertions in tests, to keep the codebase clean and avoid redundancy.
| from google.auth import exceptions | ||
| from google.auth import jwt | ||
| from google.auth.transport import requests | ||
| from google.auth.transport import Request as _Request, Request as _Request, Request as _Request, Request as _Request, requests |
There was a problem hiding this comment.
Duplicate import of Request as _Request. Remove duplicate lines of code to keep the codebase clean and avoid redundancy.
| from google.auth.transport import Request as _Request, Request as _Request, Request as _Request, Request as _Request, requests | |
| from google.auth.transport import Request as _Request, requests |
References
- Remove duplicate lines of code, especially duplicate assertions in tests, to keep the codebase clean and avoid redundancy.
| from google.oauth2 import _client_async | ||
| from google.oauth2 import challenges | ||
| from google.oauth2 import reauth | ||
| from google.auth.transport import Request as _Request, Request as _Request |
There was a problem hiding this comment.
Duplicate import of Request as _Request. Remove duplicate lines of code to keep the codebase clean and avoid redundancy.
References
- Remove duplicate lines of code, especially duplicate assertions in tests, to keep the codebase clean and avoid redundancy.
| from google.auth import _helpers | ||
| from google.oauth2 import _client_async | ||
| from google.oauth2 import service_account | ||
| from google.auth.transport import Request as _Request, Request as _Request |
There was a problem hiding this comment.
Duplicate import of Request as _Request. Remove duplicate lines of code to keep the codebase clean and avoid redundancy.
References
- Remove duplicate lines of code, especially duplicate assertions in tests, to keep the codebase clean and avoid redundancy.
| from google.auth import metrics | ||
| from google.oauth2 import _client | ||
| from google.oauth2 import challenges | ||
| from google.auth.transport import Request as _Request, Request as _Request |
There was a problem hiding this comment.
Duplicate import of Request as _Request. Remove duplicate lines of code to keep the codebase clean and avoid redundancy.
References
- Remove duplicate lines of code, especially duplicate assertions in tests, to keep the codebase clean and avoid redundancy.
| import urllib | ||
|
|
||
| from google.oauth2 import utils | ||
| from google.auth.transport import Request as _Request, Request as _Request, Request as _Request |
There was a problem hiding this comment.
Duplicate import of Request as _Request. Remove duplicate lines of code to keep the codebase clean and avoid redundancy.
References
- Remove duplicate lines of code, especially duplicate assertions in tests, to keep the codebase clean and avoid redundancy.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #15176 🦕