fix: improve type checking by daniel-sanche · Pull Request #2530 · googleapis/gapic-generator-python · GitHub
Skip to content
This repository was archived by the owner on Mar 26, 2026. It is now read-only.

fix: improve type checking#2530

Merged
daniel-sanche merged 38 commits into
mainfrom
unpin_mypy
Mar 6, 2026
Merged

fix: improve type checking#2530
daniel-sanche merged 38 commits into
mainfrom
unpin_mypy

Conversation

@daniel-sanche

Copy link
Copy Markdown
Contributor

Fixes #2410

We currently have mypy pinned to an older version, because it fails on newer ones

This PR unpins the dependency to use the latest version, and fixes the problems it finds:

  • the *_operation methods in _mixins accept None values, but don't properly handle the None case
  • _get_default_mtls_endpoint doesn't handle what happens if the regex check returns None
  • the logger interceptor converts bytes to strings, without being explicit about the expected format
  • various type annotations didn't match the actual return type

@product-auto-label product-auto-label Bot added the size: m Pull request size is medium. label Jan 21, 2026
@gemini-code-assist

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request successfully addresses the issues identified with mypy by unpinning its version and resolving the type-related problems it exposed. The changes include adding type hints for better clarity, handling None values in request objects, and improving logging for debugging. The addition of --check-untyped-defs to the mypy configuration enhances the strictness of type checking, which is a positive step for code quality. Overall, the changes improve the robustness and maintainability of the generated code.

Comment on lines +383 to +384
if request is None:
request = {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Initializing request to an empty dictionary when it's None is a good practice. This ensures that the subsequent isinstance(request, dict) check and keyword expansion for protobuf message construction work as expected, preventing potential TypeError or AttributeError if None was passed directly.

Comment on lines +148 to +149
if request is None:
request = {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Initializing request to an empty dictionary when it's None is a good practice. This ensures that the subsequent isinstance(request, dict) check and keyword expansion for protobuf message construction work as expected, preventing potential TypeError or AttributeError if None was passed directly.

Comment on lines +200 to +201
if request is None:
request = {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Initializing request to an empty dictionary when it's None is a good practice. This ensures that the subsequent isinstance(request, dict) check and keyword expansion for protobuf message construction work as expected, preventing potential TypeError or AttributeError if None was passed directly.

Comment on lines +630 to +631
if request is None:
request = {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Initializing request to an empty dictionary when it's None is a good practice. This ensures that the subsequent isinstance(request, dict) check and keyword expansion for protobuf message construction work as expected, preventing potential TypeError or AttributeError if None was passed directly.

Comment on lines +687 to +688
if request is None:
request = {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Initializing request to an empty dictionary when it's None is a good practice. This ensures that the subsequent isinstance(request, dict) check and keyword expansion for protobuf message construction work as expected, preventing potential TypeError or AttributeError if None was passed directly.

Comment on lines +29 to +30
if request is None:
request = {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Initializing request to an empty dictionary when it's None is a good practice. This ensures that the subsequent isinstance(request, dict) check and keyword expansion for protobuf message construction work as expected, preventing potential TypeError or AttributeError if None was passed directly.

Comment on lines +107 to +108
"--check-untyped-defs",
*session.posargs,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Adding --check-untyped-defs to the mypy command enforces stricter type checking, which is a good practice for maintaining high code quality and catching potential type-related bugs early. Including *session.posargs also makes the nox session more flexible for custom mypy runs.

@product-auto-label product-auto-label Bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Jan 21, 2026
@product-auto-label product-auto-label Bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Jan 21, 2026
@daniel-sanche daniel-sanche marked this pull request as ready for review January 21, 2026 17:26
@daniel-sanche daniel-sanche requested a review from a team January 21, 2026 17:26
@daniel-sanche daniel-sanche changed the title [DRAFT] fix: improve type checking fix: improve type checking Jan 21, 2026
@daniel-sanche daniel-sanche changed the title fix: improve type checking [DRAFT] fix: improve type checking Feb 4, 2026
@daniel-sanche daniel-sanche marked this pull request as draft February 4, 2026 23:17
@daniel-sanche daniel-sanche changed the title [DRAFT] fix: improve type checking fix: improve type checking Feb 7, 2026
@daniel-sanche daniel-sanche marked this pull request as ready for review February 7, 2026 08:52
@daniel-sanche daniel-sanche requested a review from a team as a code owner February 7, 2026 08:52

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There should be a blank line before this decorator to separate the tests.

This happens multiple times in the PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch. It looks like this file was generating other methods with that same problem, even before this PR. I'm kind of surprised it's not raising any errors...

But I updated the template, so this shouldn't be a problem now

@chalmerlowe chalmerlowe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have one comment: it looks like something is off in one of the templates OR the generation code block that is causing a test to appear immediately after another test with no intervening blank line.

@Linchin Linchin left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good to me, seems mostly to be type annotation improvements with some additional unit test templates. But I think it makes sense to let someone with more gapic expertise to double check the tests.

@daniel-sanche daniel-sanche merged commit bc3836b into main Mar 6, 2026
113 checks passed
@daniel-sanche daniel-sanche deleted the unpin_mypy branch March 6, 2026 19:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

size: xl Pull request size is extra large.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mypy check fails in goldens-lint presubmit as of mypy 1.16.0

4 participants