Add CST-based script to complete missing async methods#36
Conversation
Summary of ChangesHello @jason810496, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request is a great step towards improving maintainability by introducing a CST-based script to automatically generate missing asynchronous methods. The refactoring to separate synchronous and asynchronous execution logic in PGMQueue is a solid design choice. However, I've identified a few critical issues in pgmq_sqlalchemy/queue.py where synchronous methods are incorrectly used in asynchronous contexts, which would cause runtime failures. I've provided detailed comments and code suggestions to address these. Additionally, I've pointed out a minor typo in a docstring. With these fixes, this will be an excellent contribution.
| def _check_pgmq_ext(self) -> None: | ||
| """Check if the pgmq extension exists.""" | ||
| if self.is_async: | ||
| return self.loop.run_until_complete(self._check_pgmq_ext_async()) | ||
| return self._check_pgmq_ext_sync() | ||
|
|
||
| async def _check_pg_partman_ext_async(self) -> None: | ||
| """Check if the pg_partman extension exists.""" | ||
| async with self.session_maker() as session: | ||
| await PGMQOperation.check_pg_partman_ext_async(session=session, commit=True) | ||
|
|
||
| def _check_pg_partman_ext_sync(self) -> None: | ||
| """Check if the pg_partman extension exists.""" | ||
| with self.session_maker() as session: | ||
| PGMQOperation.check_pg_partman_ext(session=session, commit=True) | ||
| self._execute_operation(PGMQOperation.check_pgmq_ext, session=None, commit=True) |
There was a problem hiding this comment.
This implementation of _check_pgmq_ext is synchronous and will fail when PGMQueue is used with an async engine, as it's called from __init__. It should handle both sync and async cases to prevent a TypeError when trying to use an async session maker in a synchronous context.
| def _check_pgmq_ext(self) -> None: | |
| """Check if the pgmq extension exists.""" | |
| if self.is_async: | |
| return self.loop.run_until_complete(self._check_pgmq_ext_async()) | |
| return self._check_pgmq_ext_sync() | |
| async def _check_pg_partman_ext_async(self) -> None: | |
| """Check if the pg_partman extension exists.""" | |
| async with self.session_maker() as session: | |
| await PGMQOperation.check_pg_partman_ext_async(session=session, commit=True) | |
| def _check_pg_partman_ext_sync(self) -> None: | |
| """Check if the pg_partman extension exists.""" | |
| with self.session_maker() as session: | |
| PGMQOperation.check_pg_partman_ext(session=session, commit=True) | |
| self._execute_operation(PGMQOperation.check_pgmq_ext, session=None, commit=True) | |
| def _check_pgmq_ext(self) -> None: | |
| """Check if the pgmq extension exists.""" | |
| if self.is_async: | |
| self.loop.run_until_complete(self._execute_async_operation(PGMQOperation.check_pgmq_ext_async, session=None, commit=True)) | |
| else: | |
| self._execute_operation(PGMQOperation.check_pgmq_ext, session=None, commit=True) |
| def _check_pg_partman_ext(self) -> None: | ||
| """Check if the pg_partman extension exists.""" | ||
| if self.is_pg_partman_ext_checked: | ||
| return | ||
| self.is_pg_partman_ext_checked | ||
|
|
||
| if self.is_async: | ||
| return self.loop.run_until_complete(self._check_pg_partman_ext_async()) | ||
| return self._check_pg_partman_ext_sync() | ||
| self._execute_operation( | ||
| PGMQOperation.check_pg_partman_ext, session=None, commit=True | ||
| ) |
There was a problem hiding this comment.
This synchronous method is called from async methods (create_partitioned_queue_async on line 345, drop_queue_async on line 453), which will cause issues with async engines. An asynchronous version of this check is needed.
I suggest adding an _check_pg_partman_ext_async method. You will then need to update create_partitioned_queue_async and drop_queue_async to call await self._check_pg_partman_ext_async().
def _check_pg_partman_ext(self) -> None:
"""Check if the pg_partman extension exists."""
self._execute_operation(
PGMQOperation.check_pg_partman_ext, session=None, commit=True
)
async def _check_pg_partman_ext_async(self) -> None:
"""Check if the pg_partman extension exists (async)."""
await self._execute_async_operation(
PGMQOperation.check_pg_partman_ext_async, session=None, commit=True
)|
|
||
| """ | ||
| # check if the pg_partman extension exists before creating a partitioned queue at runtime | ||
| self._check_pg_partman_ext() |
| """ | ||
| # check if the pg_partman extension exists before dropping a partitioned queue at runtime | ||
| if partitioned: | ||
| self._check_pg_partman_ext() |
|
|
||
| # set exponential backoff retry | ||
| await pgmq_client.set_vt_async( | ||
| queue_name=query_name, |
jason810496
left a comment
There was a problem hiding this comment.
@copilot Add compelete_missing_async_methods_for_operation.py and take scripts/compelete_missing_async_methods_for_queue.py as reference.
|
@jason810496 I've opened a new pull request, #37, to work on those changes. Once the pull request is ready, I'll request review from you. |
jason810496
left a comment
There was a problem hiding this comment.
@copilot Add compelete_missing_test_for_queue.py and take scripts/compelete_missing_async_methods_for_queue.py as reference and use CST-based approach.
|
@jason810496 I've opened a new pull request, #38, to work on those changes. Once the pull request is ready, I'll request review from you. |
jason810496
left a comment
There was a problem hiding this comment.
@copilot Add compelete_missing_test_for_operation.py and take scripts/compelete_missing_async_methods_for_queue.py as reference and use CST-based approach.
|
@jason810496 I've opened a new pull request, #39, to work on those changes. Once the pull request is ready, I'll request review from you. |
jason810496
left a comment
There was a problem hiding this comment.
@copilot fix the test by analyzing the log of https://github.com/jason810496/pgmq-sqlalchemy/actions/runs/20721959215/job/59487785110?pr=36
|
@jason810496 I've opened a new pull request, #40, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@copilot Make Or for the Check which approach have the minimum change. |
|
@jason810496 I've opened a new pull request, #41, to work on those changes. Once the pull request is ready, I'll request review from you. |
145ba4b to
da8aba2
Compare
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
Distinguish sync and async operations in PGMQueue - Introduced a pre-commit hook to check for missing async methods in PGMQueue. - Added scripts to identify and generate missing async methods. - Created utility functions for AST manipulation and method transformation. - Established configuration for project paths and console output.
* Initial plan * Refactor AST-based code to use libcst for better code transformation Co-authored-by: jason810496 <[email protected]> * Fix: only wrap call expressions in await, not literals Co-authored-by: jason810496 <[email protected]> * Ask whether to apply change * Apply missing async methods * Correct files for check-sync-async-method-for-queue pre-commit * Add check for operation as well --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jason810496 <[email protected]> Co-authored-by: LIU ZHE YOU <[email protected]>
* Initial plan * Add compelete_missing_async_methods_for_operation.py script and operation_ast helper Co-authored-by: jason810496 <[email protected]> * Fix path resolution bug in operation_ast.py Co-authored-by: jason810496 <[email protected]> * Address code review feedback: improve docstring handling and remove unnecessary method Co-authored-by: jason810496 <[email protected]> * Improve docstring handling robustness with better length checks Co-authored-by: jason810496 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jason810496 <[email protected]>
da8aba2 to
9e76606
Compare
#39) * Add pre-commit hooks and scripts for async method checks in PGMQueue Distinguish sync and async operations in PGMQueue - Introduced a pre-commit hook to check for missing async methods in PGMQueue. - Added scripts to identify and generate missing async methods. - Created utility functions for AST manipulation and method transformation. - Established configuration for project paths and console output. * Refactor code transformation scripts from ast to libcst (#35) * Initial plan * Refactor AST-based code to use libcst for better code transformation Co-authored-by: jason810496 <[email protected]> * Fix: only wrap call expressions in await, not literals Co-authored-by: jason810496 <[email protected]> * Ask whether to apply change * Apply missing async methods * Correct files for check-sync-async-method-for-queue pre-commit * Add check for operation as well --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jason810496 <[email protected]> Co-authored-by: LIU ZHE YOU <[email protected]> * Initial plan * Add compelete_missing_test_for_operation.py script with CST-based approach Co-authored-by: jason810496 <[email protected]> * Fix code review feedback: remove redundant code and use Tuple from typing Co-authored-by: jason810496 <[email protected]> * Finalize test_operation * Fix _check_pg_partman_ext naming --------- Co-authored-by: LIU ZHE YOU <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: jason810496 <[email protected]>
…38) * Add pre-commit hooks and scripts for async method checks in PGMQueue Distinguish sync and async operations in PGMQueue - Introduced a pre-commit hook to check for missing async methods in PGMQueue. - Added scripts to identify and generate missing async methods. - Created utility functions for AST manipulation and method transformation. - Established configuration for project paths and console output. * Refactor code transformation scripts from ast to libcst (#35) * Initial plan * Refactor AST-based code to use libcst for better code transformation Co-authored-by: jason810496 <[email protected]> * Fix: only wrap call expressions in await, not literals Co-authored-by: jason810496 <[email protected]> * Ask whether to apply change * Apply missing async methods * Correct files for check-sync-async-method-for-queue pre-commit * Add check for operation as well --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jason810496 <[email protected]> Co-authored-by: LIU ZHE YOU <[email protected]> * Initial plan * Add complete_missing_test_for_queue.py script with CST-based approach Co-authored-by: jason810496 <[email protected]> * Apply ruff formatting to test generation scripts Co-authored-by: jason810496 <[email protected]> * Address code review feedback: fix path resolution, type hints, and documentation Co-authored-by: jason810496 <[email protected]> * Fix missing async replacment for compelete test queue * Fix imports for async fixtures --------- Co-authored-by: LIU ZHE YOU <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: jason810496 <[email protected]>
No description provided.