Skip to content

feat(gapic-generator): add CodeWriter pure-python codegen utility#17638

Draft
ohmayr wants to merge 1 commit into
mainfrom
feature/codegen-writer
Draft

feat(gapic-generator): add CodeWriter pure-python codegen utility#17638
ohmayr wants to merge 1 commit into
mainfrom
feature/codegen-writer

Conversation

@ohmayr

@ohmayr ohmayr commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@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 introduces a new CodeWriter class to generate structured Python code programmatically, replacing Jinja templates, and includes comprehensive unit tests. The review feedback recommends moving the textwrap import to the top of the file to adhere to PEP-8, sorting generated imports within groups (straight imports before from-imports), and updating the corresponding unit tests to match the sorted output.

Comment on lines +89 to +90
import textwrap
cleaned = textwrap.dedent(text).strip("\n")

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

According to PEP-8, imports should be placed at the top of the file rather than inside functions, unless there is a specific reason (such as avoiding circular dependencies or heavy import costs). Since textwrap is a lightweight standard library module, please move import textwrap to the top of the file.

Suggested change
import textwrap
cleaned = textwrap.dedent(text).strip("\n")
cleaned = textwrap.dedent(text).strip("\n")
References
  1. Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants. (link)

Comment on lines +101 to +113
groups = [g for g in (std, third_party, local) if g]
for i, group in enumerate(groups):
for item in group:
if isinstance(item, str):
self.write_line(
item if item.startswith("import ") or item.startswith("from ") else f"import {item}"
)
elif isinstance(item, tuple):
module, symbols = item
sym_str = ", ".join(sorted(symbols))
self.write_line(f"from {module} import {sym_str}")
if i < len(groups) - 1:
self.newline()

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

The docstring for write_imports claims to emit PEP-8 compliant grouped import statements, but the imports within each group are currently written in insertion order rather than being sorted. To ensure PEP-8 compliance automatically, we should sort the imports within each group (straight imports first, then from-imports, both sorted alphabetically).

Suggested change
groups = [g for g in (std, third_party, local) if g]
for i, group in enumerate(groups):
for item in group:
if isinstance(item, str):
self.write_line(
item if item.startswith("import ") or item.startswith("from ") else f"import {item}"
)
elif isinstance(item, tuple):
module, symbols = item
sym_str = ", ".join(sorted(symbols))
self.write_line(f"from {module} import {sym_str}")
if i < len(groups) - 1:
self.newline()
groups = [g for g in (std, third_party, local) if g]
for i, group in enumerate(groups):
formatted = []
for item in group:
if isinstance(item, str):
formatted.append(
item if item.startswith("import ") or item.startswith("from ") else f"import {item}"
)
elif isinstance(item, tuple):
module, symbols = item
sym_str = ", ".join(sorted(symbols))
formatted.append(f"from {module} import {sym_str}")
# Sort imports: straight imports first, then from-imports, both alphabetically
for stmt in sorted(formatted, key=lambda s: (1, s) if s.startswith("from ") else (0, s)):
self.write_line(stmt)
if i < len(groups) - 1:
self.newline()
References
  1. Within each group, imports should be sorted. (link)

Comment on lines +95 to +103
expected = (
"from collections import OrderedDict\n"
"import json\n"
"from typing import Callable, Dict, List\n"
"\n"
"from google.api_core import gapic_v1\n"
"\n"
"from .transports.base import FooTransport\n"
)

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

Update the expected imports output to match the new sorted behavior where straight imports are grouped before from-imports and sorted alphabetically.

Suggested change
expected = (
"from collections import OrderedDict\n"
"import json\n"
"from typing import Callable, Dict, List\n"
"\n"
"from google.api_core import gapic_v1\n"
"\n"
"from .transports.base import FooTransport\n"
)
expected = (
"import json\n"
"from collections import OrderedDict\n"
"from typing import Callable, Dict, List\n"
"\n"
"from google.api_core import gapic_v1\n"
"\n"
"from .transports.base import FooTransport\n"
)

@ohmayr ohmayr force-pushed the feature/codegen-writer branch 2 times, most recently from 2c39683 to 7cbcd5a Compare July 7, 2026 02:19
@ohmayr ohmayr force-pushed the feature/codegen-writer branch from 7cbcd5a to c8a8ee9 Compare July 7, 2026 04:05
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.

1 participant