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

fix: add type hints to credentials #1605

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

rinarakaki
Copy link

@rinarakaki rinarakaki commented Oct 11, 2024

Enables googleapis/python-cloud-core#302

I checked:

  • pyright google/auth/_credentials_base.py
  • pyright google/auth/_default.py
  • pyright google/auth/_refresh_worker.py
  • pyright google/auth/aio/credentials.py
  • pyright google/auth/credentials.py
  • pyright google/auth/metrics.py
  • pyright google/oauth2/service_account.py

@rinarakaki rinarakaki requested review from a team as code owners October 11, 2024 13:58
@parthea parthea requested a review from ohmayr October 11, 2024 14:20

from google.auth import _helpers


class _BaseCredentials(metaclass=abc.ABCMeta):
class BaseCredentials(metaclass=abc.ABCMeta):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we want to change this to a public class?

Copy link
Author

Choose a reason for hiding this comment

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

_BaseCredentials is imported from outside the _credentials_base.py file, so it's not private in that scope. You can see that it's captured as an error by running:

pyright google/auth/credentials.py

@@ -62,7 +63,7 @@ def refresh(self, request):
# (pylint doesn't recognize that this is abstract)
raise NotImplementedError("Refresh must be implemented")

def _apply(self, headers, token=None):
def _apply(self, headers: dict[str, str], token: Optional[str] = None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we use Mapping from typing (here and in other places)? Also update the docstring to Mapping[str, str].

Copy link
Author

Choose a reason for hiding this comment

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

This operation requires the type of headers has __setitem__ method that's why being a Mapping is not enough:

headers["authorization"] = "Bearer {}".format(
    _helpers.from_bytes(token or self.token)
)

Copy link
Author

Choose a reason for hiding this comment

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

Do you want me to modify the doctring instead?

@@ -32,7 +34,7 @@ def __init__(self):
self._worker = None
self._lock = threading.Lock() # protects access to worker threads.

def start_refresh(self, cred, request):
def start_refresh(self, cred: Credentials, request: Request):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def start_refresh(self, cred: Credentials, request: Request):
def start_refresh(self, cred: Credentials, request: Request): -> bool

Copy link
Author

Choose a reason for hiding this comment

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

done

@@ -61,8 +63,8 @@ def start_refresh(self, cred, request):

def clear_error(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def clear_error(self):
def clear_error(self): -> None

Copy link
Author

Choose a reason for hiding this comment

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

done

from google.auth._refresh_worker import RefreshThreadManager
from google.auth.credentials import Credentials
Copy link
Contributor

Choose a reason for hiding this comment

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

We're importing from the file itself? This doesn't seem right.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from google.auth.credentials import Credentials

Copy link
Author

Choose a reason for hiding this comment

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

Ah, yeah, sorry you're right

@ohmayr ohmayr changed the title chore: add typing chore: add type hints to credentials Oct 11, 2024
@parthea parthea changed the title chore: add type hints to credentials fix: add type hints to credentials Oct 11, 2024
@rinarakaki rinarakaki requested a review from ohmayr October 15, 2024 01:12
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.

2 participants