fix: make Batch naming resource plural aware#1573
Conversation
Summary of ChangesHello @noahdietz, 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 refactors the linting logic for "Batch" methods (e.g., BatchGet, BatchCreate, BatchUpdate, BatchDelete) to ensure their names correctly reflect plural resources. By centralizing the pluralization check into a new common utility function, the linter can now intelligently determine the correct plural form by first consulting 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 is a great improvement, centralizing the plural method naming check into a common, resource-aware lint function. This refactoring enhances maintainability and correctness by using the plural property from resource definitions. The addition of comprehensive test cases is also a valuable contribution. I have a couple of suggestions for the new LintPluralMethodName function to further improve readability and performance.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great improvement. It centralizes the logic for checking plural method names for batch operations into a new, resource-aware utility function LintPluralMethodName. This refactoring significantly reduces code duplication and enhances the linter's accuracy by leveraging resource definitions when available. The accompanying test updates are thorough and cover the new functionality well. I've identified one potential high-severity issue in the new utility function that could lead to a panic, for which I've provided a suggestion.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great improvement. It centralizes the logic for checking plural method names for batch operations into a new utility function, utils.LintPluralMethodName. This new function is also resource-aware, prioritizing the plural field from the resource definition (AIP-123) when available, which improves the linter's accuracy. The refactoring is applied consistently across all relevant batch method rules (AIP-231, 233, 234, 235), and the changes are well-supported by new and updated tests. The code quality is high, and I have only one minor suggestion.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
2 participants
Centralizes "plural method naming" check for collection-based methods, explicitly Batch methods e.g. AIP-231, into a common lint. Makes that common lint "resource aware" in that it checks if the resource in the response (if present) has the AIP-123
pluralproperty set, then bases its method plural naming check on that value instead of thepluralizelibrary.Adds a bunch of test cases to each rule and the new common lint method.
Note: BatchDelete is not covered in the updated heuristics b.c the response is typically
google.protobuf.Emptyand the only reference to the resource being deleted is theresource_referenceon the request message fieldrepeated names. This should be updated later, but it is not urgent.Updates #1566