Refactor async generator #226
Conversation
Summary by CodeRabbit
WalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| # Run Ruff for code formatting, providing the same configuration as the project. | ||
| shutil.copy(PYPROJECT_TOML, f"{target}") | ||
| subprocess.call(["ruff", "format", target]) | ||
| subprocess.call(["ruff", "check", "--fix", target]) | ||
| Path(f"{target}/pyproject.toml").unlink() | ||
|
|
There was a problem hiding this comment.
I've removed this, because the catch-all poe format formats the code anyway.
There was a problem hiding this comment.
I see that removing this is not advised, as otherwise the check subcommand would fail. Reverting.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
script/generate_async.py (1)
100-103:str.replace()-based import rewriting risks touching false positivesUsing plain
str.replace()can unintentionally rewrite occurrences inside comments, doc-strings or string literals that just happen to contain the same text.
A small regex anchored to the start of a line (^ *from …) avoids that risk while remaining simple.-for relative_import in [".base", "..client", "..knowledge", "..model"]: - module_dump = module_dump.replace(f"from {relative_import}", f"from .{relative_import}") +pattern = re.compile(rf'^(\s*)from ({"|".join(map(re.escape, [".base", "..client", "..knowledge", "..model"]))})\b', flags=re.MULTILINE) +module_dump = pattern.sub(lambda m: f'{m.group(1)}from .{m.group(2)}', module_dump)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyproject.toml(0 hunks)script/generate_async.py(1 hunks)
💤 Files with no reviewable changes (1)
- pyproject.toml
🔇 Additional comments (1)
script/generate_async.py (1)
112-114: Verify thatself.api.versionis awaitableBlindly turning an attribute access into
await self.api.versionassumes the property returns a coroutine.
If it returns a plain string/int, the generated code will raiseTypeError: object str is not awaitable.Please confirm the type of
self.api.version; if it’s synchronous, this replacement should be removed (or turned into an async call such asawait self.api.version()if that’s the real intent).
| # Modify function definitions. | ||
| module_dump = re.sub(r"( {4}def )(?!_)", r" async def ", module_dump) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Function-definition regex misses indents ≠ 4 & may hit code snippets
- It only matches exactly four leading spaces – methods nested deeper (e.g. inside an inner class) stay synchronous.
- The pattern isn’t anchored to the line start so a
" def foo"string in a doc-string would also be changed.
-module_dump = re.sub(r"( {4}def )(?!_)", r" async def ", module_dump)
+module_dump = re.sub(
+ r'^(\s{4,})def (?!_)', # any indent ≥4 at BOL, next char not “_”
+ r'\1async def ',
+ module_dump,
+ flags=re.MULTILINE,
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Modify function definitions. | |
| module_dump = re.sub(r"( {4}def )(?!_)", r" async def ", module_dump) | |
| # Modify function definitions. | |
| module_dump = re.sub( | |
| r'^(\s{4,})def (?!_)', # any indent ≥4 at BOL, next char not “_” | |
| r'\1async def ', | |
| module_dump, | |
| flags=re.MULTILINE, | |
| ) |
🤖 Prompt for AI Agents
In script/generate_async.py around lines 104 to 106, the regex for modifying
function definitions only matches lines with exactly four leading spaces and is
not anchored to the start of the line, causing it to miss nested methods and
potentially alter strings in doc-strings. Update the regex to anchor it to the
start of the line and allow matching any number of leading spaces before "def"
to correctly identify all function definitions regardless of indentation level,
while avoiding changes inside strings or comments.
| module_dump = re.sub(r"self\.client\.(.+)\(", r"await self.client.\1(", module_dump) | ||
| module_dump = re.sub(r"= self\.(.+)\(", r"= await self.\1(", module_dump) | ||
| module_dump = re.sub(r"send_request\(", r"await send_request(", module_dump) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Greedy .+ may over-capture; tighten the call-site patterns
The current patterns can unexpectedly swallow dots or spaces until the next “(”.
Limiting the capture to identifier characters removes that ambiguity and avoids matching across comments.
-module_dump = re.sub(r"self\.client\.(.+)\(", r"await self.client.\1(", module_dump)
-module_dump = re.sub(r"= self\.(.+)\(", r"= await self.\1(", module_dump)
+module_dump = re.sub(r"self\.client\.([A-Za-z_][A-Za-z0-9_]*)\(", r"await self.client.\1(", module_dump)
+module_dump = re.sub(r"= (\s*)self\.([A-Za-z_][A-Za-z0-9_]*)\(", r"= \1await self.\2(", module_dump)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| module_dump = re.sub(r"self\.client\.(.+)\(", r"await self.client.\1(", module_dump) | |
| module_dump = re.sub(r"= self\.(.+)\(", r"= await self.\1(", module_dump) | |
| module_dump = re.sub(r"send_request\(", r"await send_request(", module_dump) | |
| module_dump = re.sub(r"self\.client\.([A-Za-z_][A-Za-z0-9_]*)\(", r"await self.client.\1(", module_dump) | |
| module_dump = re.sub(r"= (\s*)self\.([A-Za-z_][A-Za-z0-9_]*)\(", r"= \1await self.\2(", module_dump) | |
| module_dump = re.sub(r"send_request\(", r"await send_request(", module_dump) |
🤖 Prompt for AI Agents
In script/generate_async.py around lines 108 to 111, the regex patterns use
greedy .+ which can over-capture and match unintended characters. Replace .+
with a more precise pattern that matches only valid identifier characters (e.g.,
\w+) to ensure the substitutions only target method names without including
dots, spaces, or other characters. Update all three re.sub calls accordingly to
use this tightened pattern.
amotl
force-pushed
the
async-generator-refactor
branch
from
9ce0645 to
ab6a2d0
Compare
1 participant
Just a few cleanups and inline comments.