Updated discovery change diff #14958
Conversation
Summary of ChangesHello @ohmayr, 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 primarily focuses on internal generator tooling improvements and a version update for the Highlights
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces performance logging to the .generator/cli.py script by adding a track_time decorator and applying it to several functions, along with enabling performance logs via a Dockerfile environment variable. It also adds ruff as a dependency and updates the __version__ for the google-cloud-discoveryengine library. Numerous docstrings across various service client files were modified to use an updated Sphinx cross-reference format and improve line wrapping. Review comments indicate that logging.basicConfig() should not be called at the module level, the functools import should be moved to the top of the file, and the ruff dependency should be pinned to a specific version. A significant concern is that the existing isort and black formatting steps were removed and replaced with a commented-out ruff block, which currently leaves the code unformatted. Additionally, a newline character should be added at the end of the requirements.in file.
| import build.util | ||
| import parse_googleapis_content | ||
| | ||
| logging.basicConfig(stream=sys.stdout, level=logging.INFO) |
There was a problem hiding this comment.
Calling logging.basicConfig() at the module level is generally discouraged because it can interfere with the logging configuration of other modules that import this one. It's better to move this configuration into a main function or an if __name__ == '__main__': block to ensure it only runs when the script is executed directly.
| os.chdir(output) | ||
| path_to_library = f"packages/{library_id}" if is_mono_repo else "." | ||
| logger.info("Running Python post-processor...") | ||
| | ||
| # 1. Run Synthtool (Templates & Fixers only) | ||
| # Note: This relies on 'nox' being disabled in your environment (via run_fast.sh shim) | ||
| # to avoid the slow formatting step inside owlbot. | ||
| logger.info("Running Python post-processor (Templates & Fixers)...") | ||
| if SYNTHTOOL_INSTALLED: | ||
| if is_mono_repo: | ||
| python_mono_repo.owlbot_main(path_to_library) | ||
| else: | ||
| # Some repositories have customizations in `librarian.py`. | ||
| # If this file exists, run those customizations instead of `owlbot_main` | ||
| if Path(f"{output}/librarian.py").exists(): | ||
| subprocess.run(["python3.14", f"{output}/librarian.py"]) | ||
| try: | ||
| if is_mono_repo: | ||
| python_mono_repo.owlbot_main(path_to_library) | ||
| else: | ||
| python.owlbot_main() | ||
| else: | ||
| raise SYNTHTOOL_IMPORT_ERROR # pragma: NO COVER | ||
| | ||
| # If there is no noxfile, run `isort`` and `black` on the output. | ||
| # This is required for proto-only libraries which are not GAPIC. | ||
| if not Path(f"{output}/{path_to_library}/noxfile.py").exists(): | ||
| subprocess.run(["isort", output]) | ||
| subprocess.run(["black", output]) | ||
| # Handle custom librarian scripts if present | ||
| if Path(f"{output}/librarian.py").exists(): | ||
| subprocess.run(["python3.14", f"{output}/librarian.py"]) | ||
| else: | ||
| python.owlbot_main() | ||
| except Exception as e: | ||
| logger.warning(f"Synthtool warning (non-fatal): {e}") | ||
| | ||
| # 2. Run RUFF (Fast Formatter & Import Sorter) | ||
| # This replaces both 'isort' and 'black' and runs in < 1 second. | ||
| # We hardcode flags here to match Black defaults so you don't need config files. | ||
| # logger.info("🚀 Running Ruff (Fast Formatter)...") | ||
| # try: | ||
| # # STEP A: Fix Imports (like isort) | ||
| # subprocess.run( | ||
| # [ | ||
| # "ruff", "check", | ||
| # "--select", "I", # Only run Import sorting rules | ||
| # "--fix", # Auto-fix them | ||
| # "--line-length=88", # Match Black default | ||
| # "--known-first-party=google", # Prevent 'google' moving to 3rd party block | ||
| # output | ||
| # ], | ||
| # check=False, | ||
| # stdout=subprocess.DEVNULL, | ||
| # stderr=subprocess.DEVNULL | ||
| # ) | ||
| | ||
| # # STEP B: Format Code (like black) | ||
| # subprocess.run( | ||
| # [ | ||
| # "ruff", "format", | ||
| # "--line-length=88", # Match Black default | ||
| # output | ||
| # ], | ||
| # check=False, | ||
| # stdout=subprocess.DEVNULL, | ||
| # stderr=subprocess.DEVNULL | ||
| # ) | ||
| # except FileNotFoundError: | ||
| # logger.warning("⚠️ Ruff binary not found. Code will be unformatted.") | ||
| # logger.warning(" Please run: pip install ruff") | ||
| | ||
| logger.info("Python post-processor ran successfully.") |
There was a problem hiding this comment.
This change removes the existing isort and black formatting step and replaces it with a commented-out block for ruff. This means that currently no formatting will be applied, which seems like an unintentional regression. If ruff is not ready to be used, the old formatting logic should be kept. If ruff is ready, this block should be uncommented.
| | ||
| logging.basicConfig(stream=sys.stdout, level=logging.INFO) | ||
| | ||
| import functools |
There was a problem hiding this comment.
To adhere to PEP 8, this import should be moved to the top of the file with other standard library imports.
| build | ||
| black==23.7.0 | ||
| isort==5.11.0 | ||
| ruff |
There was a problem hiding this comment.
To ensure reproducible builds, it's best practice to pin dependencies to a specific version. Please consider pinning ruff to a specific version (e.g., ruff==0.1.6). Also, it's a good practice to end files with a newline character.
| "packages/google-cloud-discoveryengine/" | ||
| ], | ||
| "tag_format": "{id}-v{version}" | ||
| } No newline at end of file |
There was a problem hiding this comment.
It's a common convention for text files to end with a newline character. Please add a newline at the end of this file.
2 participants
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕