Conversation
![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 implements logic to exclude 'vertexai' artifacts from version update comments in both parent and BOM POM files and adds a unit test for the BOM update. Feedback includes suggestions to refactor the duplicated exclusion logic, add test coverage for the parent POM update, and move inline imports in the test file to the top to comply with PEP 8.
| new_dependency.append(new_artifact) | ||
| new_dependency.append(new_version) | ||
| new_dependency.append(comment) | ||
| if "vertexai" not in m.artifact_id: |
There was a problem hiding this comment.
The logic to exclude 'vertexai' artifacts from receiving 'release please' comments is duplicated here and in update_bom_pom (line 314). Consider refactoring this into a shared constant or a helper function to improve maintainability and ensure consistency across different POM update functions.
| for sub_dir in sub_dirs: | ||
| self.__remove_file_in_subdir(ad_manager_resource, sub_dir) | ||
|
|
||
| def test_update_bom_pom_excludes_vertexai_comment(self): |
There was a problem hiding this comment.
The new test case test_update_bom_pom_excludes_vertexai_comment only verifies the logic for update_bom_pom. Since the exclusion logic was also added to update_parent_pom, consider adding a corresponding test case to ensure that function also correctly handles 'vertexai' artifacts.
| import tempfile | ||
| from library_generation.owlbot.src.poms.module import Module | ||
| from library_generation.owlbot.src.fix_poms import update_bom_pom |
There was a problem hiding this comment.
According to PEP 8, imports should be placed at the top of the file. Importing modules like tempfile, Module, and update_bom_pom inside a test method is generally discouraged as it can lead to redundant imports and reduced readability. Please move these to the top of the file.
References
- PEP 8: Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants. (link)
| self.assertIn("x-version-update:google-cloud-datastore:current", content) | ||
|
|
||
| finally: | ||
| import os |
There was a problem hiding this comment.
Similar to the imports at the beginning of the method, import os should be moved to the top of the file to adhere to PEP 8 standards.
References
- PEP 8: Imports are always put at the top of the file. (link)
blakeli0
deleted the
hermetic-build/omit-release-please-comment-in-vertexai
branch
2 participants
No description provided.