Skip to content

Conversation

@wyattscarpenter
Copy link

Recently, I've been using this library with mypy strict mode, which doesn't like the fact that these functions are unannotated (they're the only functions I use, and thus the only ones mypy complains about to me). I'm happy to see that this project has py.typed, so these annotations should fix the problem!

@wyattscarpenter wyattscarpenter requested review from a team as code owners August 20, 2024 08:17
@arithmetic1728 arithmetic1728 added the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 20, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 20, 2024
@arithmetic1728
Copy link
Contributor

arithmetic1728 commented Aug 20, 2024

Could you squash your commits, and change the commit message tofix: add types to verify_token and request __init__? The conventionalcommits.org test requires the commit message to start with the "fix:" prefix. Thanks!

@wyattscarpenter
Copy link
Author

Happy to!

Recently, I've been using this library with mypy strict mode, which doesn't like the fact that these functions are unannotated (they're the only functions I use, and thus the only ones mypy complains about to me). I'm happy to see that this project has py.typed, so these annotations should fix the problem!
@wyattscarpenter wyattscarpenter changed the title Add types to verify_token and Request __init__ based on comments in the source code. fix: add types to verify_token and Request __init__ based on comments in the source code. Aug 5, 2025
audience: str | list[str] | None = None,
certs_url: str = _GOOGLE_OAUTH2_CERTS_URL,
clock_skew_in_seconds: int = 0,
) -> Mapping[str, Any]:
Copy link
Author

Choose a reason for hiding this comment

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

It's actually bad to return something with Any in it, but, um, that's just what was in the comment. ¯\_(ツ)_/¯

If the reviewer happens to have a better & more-accurate idea, let him change this line accordingly.

@wyattscarpenter
Copy link
Author

I'm hoping I can get this looked at in the near future. Also, it seems like (at least this time), it's the title of the PR that has to have fix: in it for conventionalcommits.org to pass, so have done so.

@wyattscarpenter wyattscarpenter changed the title fix: add types to verify_token and Request __init__ based on comments in the source code. fix: add types to default and verify_token and Request __init__ based on comments in the source code. Aug 6, 2025
@wyattscarpenter wyattscarpenter requested a review from a team as a code owner August 6, 2025 01:42
@chalmerlowe chalmerlowe requested a review from a team as a code owner December 22, 2025 18:57
@chalmerlowe chalmerlowe self-assigned this Dec 23, 2025
@chalmerlowe chalmerlowe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 23, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 23, 2025
Copy link
Contributor

@chalmerlowe chalmerlowe left a comment

Choose a reason for hiding this comment

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

LGTM. Let's see how the tests turn out.
Kokoro and Kokoro system-3.10 were failing due to circular imports. Hopefully we can nix that using the conditional.

@yoshi-kokoro yoshi-kokoro removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Dec 23, 2025
@chalmerlowe chalmerlowe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 23, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 23, 2025
Update to deal with linting issues.
Updates to correct linting issues.
@chalmerlowe chalmerlowe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 23, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 23, 2025
fixes to deal with linting and coverage errors.
@chalmerlowe chalmerlowe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 23, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 23, 2025
missing a dadgum space character after the comment octothorpe. 🤯
@chalmerlowe chalmerlowe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 23, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 23, 2025
more linting stuff related to flake8 not liking our type hinting.
@chalmerlowe chalmerlowe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 23, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 23, 2025
Updates order of imports and removes explanatory comments. flake8 does not like them, it does not match the expected pattern for this situation.
@chalmerlowe chalmerlowe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 26, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 26, 2025
@chalmerlowe
Copy link
Contributor

This almost broke me.

For my own sake and anyone else who looks at all this commit hell:

  • blacken, flake8, mypy, coverage, and sphinx all have rules for how things should be formatted, checked, and tested. Some of those rules differ by Python Runtime and some introduce subtle interactions that caused one OR more of these utilities to fail in ways that I did not expect.

I attempted to do the following:

  • ensure that the type-hints were viable in under both Python 3.9 and 3.10+ since there was type hinting capabilities in 3.10 that was missing in 3.9
  • avoid a circular import by adding an if condition related to type checking to control when/how specific libraries were imported i.e. only during type checking, not during runtime.
  • comment the change so future me would remember why
  • remove the comment because flake8 has rigid rules about such type checking conditionals: it expects 3 lines between it and the last normal import statement: the import statement, a blank line, and the if statement. Comments added extra lines to the block and flake8 was NOT happy.
  • include full paths to the Request and Credentials type hints because sphinx saw multiple cross references to different Request and Credential types that come from different parts of the code and did not know how to cross reference them correctly.
  • add flake8 pragmas because flake8 also didn't like the fact that the imports were not used in the code (flake8 did not recognize that the if statement and code block were required for type checking... it just considered them unused)
  • reorder the import statements to put the if conditional last, since flake8 has rules about where such things can go
  • add pragmas to ensure coverage won't flag missing unittest coverage for the if conditional (we chose not to unittest it since it is used solely by the type checker).
  • ensure there are two spaces in front of the pragma because blacken requires two spaces:
    • if TYPE_CHECKING: # pragma: NO COVER
      versus
    • if TYPE_CHECKING: # pragma: NO COVER
  • add several miscellaneous commits and tests to try and discover what was happening with all of these intermingled rules, etc.

@chalmerlowe chalmerlowe added the automerge Merge the pull request once unit tests and other checks pass. label Dec 26, 2025
Copy link
Contributor

@chalmerlowe chalmerlowe left a comment

Choose a reason for hiding this comment

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

LGTM

@gcf-merge-on-green
Copy link

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Dec 27, 2025
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.

4 participants