check shell case description #33399
check shell case description #33399
Conversation
Summary of ChangesHello @minhuinie, 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 existing test file validation system by extending its capabilities to include shell scripts. It introduces a new set of functions designed to parse and enforce documentation standards for shell script docstrings, specifically those using the HERE document format. This ensures that shell scripts adhere to the same level of documentation quality and consistency as Python test files, improving maintainability and clarity across the codebase. 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 validation for docstrings in shell script test files, which is a great addition for ensuring documentation quality. The implementation is mostly solid, but I've identified a few areas for improvement. My review includes suggestions to reduce code duplication, fix a bug in the validation logic, and improve maintainability by using constants. Addressing these points will make the new validation script more robust and easier to manage in the future.
| has_errors = True | ||
| else: | ||
| # Check blank line before Description | ||
| if description_index > 1 and lines[description_index - 1].strip(): |
There was a problem hiding this comment.
There appears to be a bug in the logic for checking if there's a blank line before the 'Description:' section. The condition if description_index > 1 will not report an error if 'Description:' is on the line immediately after the summary (i.e., at index 1), which is incorrect. To correctly enforce a blank line, the condition should be if description_index > 0 and lines[description_index - 1].strip():, which is consistent with the checks for other fields.
| if description_index > 1 and lines[description_index - 1].strip(): | |
| if description_index > 0 and lines[description_index - 1].strip(): |
| def check_required_field_shell(lines, field, file_path, other_fields): | ||
| """ | ||
| 检查 shell 脚本中的必填字段（如 Since)，支持多行内容。 | ||
| """ | ||
| index = next((i for i, line in enumerate(lines) if re.match(rf'^{field}\s*:\s*.*$', line, re.IGNORECASE)), None) | ||
| has_errors = False | ||
| if index is None: | ||
| print(f"The docstring in shell script {file_path} does not contain a valid '{field}:' section.") | ||
| has_errors = True | ||
| else: | ||
| if index > 0 and lines[index - 1].strip(): | ||
| print(f"The docstring in shell script {file_path} must have a blank line before '{field}'.") | ||
| has_errors = True | ||
| value_lines = [] | ||
| first_line_value = re.sub(rf'^{field}\s*:\s*', '', lines[index], flags=re.IGNORECASE).strip() | ||
| if first_line_value: | ||
| value_lines.append(first_line_value) | ||
| for i in range(index + 1, len(lines)): | ||
| if lines[i].strip() and not re.match(rf'^({"|".join(other_fields)})\s*:\s*.*$', lines[i], re.IGNORECASE): | ||
| value_lines.append(lines[i].strip()) | ||
| else: | ||
| break | ||
| if not value_lines: | ||
| print(f"The docstring in shell script {file_path} does not contain a value for the '{field}:' field.") | ||
| has_errors = True | ||
| return has_errors | ||
|
|
||
| def check_optional_field_shell(lines, field, file_path, other_fields): | ||
| """ | ||
| 检查 shell 脚本中的可选字段（如 Labels、Jira、History, Catalog)，支持多行内容。 | ||
| """ | ||
| index = next((i for i, line in enumerate(lines) if re.match(rf'^{field}\s*:\s*.*$', line, re.IGNORECASE)), None) | ||
| has_errors = False | ||
| if index is not None: | ||
| if index > 0 and lines[index - 1].strip(): | ||
| print(f"The docstring in shell script {file_path} must have a blank line before '{field}'.") | ||
| has_errors = True | ||
| value_lines = [] | ||
| first_line_value = re.sub(rf'^{field}\s*:\s*', '', lines[index], flags=re.IGNORECASE).strip() | ||
| if first_line_value: | ||
| value_lines.append(first_line_value) | ||
| for i in range(index + 1, len(lines)): | ||
| if lines[i].strip() and not re.match(rf'^({"|".join(other_fields)})\s*:\s*.*$', lines[i], re.IGNORECASE): | ||
| value_lines.append(lines[i].strip()) | ||
| else: | ||
| break | ||
| if not value_lines: | ||
| print(f"The docstring in shell script {file_path} does not contain a value for the '{field}' field.") | ||
| has_errors = True | ||
| return has_errors |
There was a problem hiding this comment.
The functions check_required_field_shell and check_optional_field_shell are nearly identical. To improve maintainability and reduce code duplication, consider refactoring them into a single helper function. This helper could take an is_required boolean parameter to handle the difference in behavior when a field is missing. The core logic for checking blank lines and parsing multi-line values can be shared.
| if first_line_value: | ||
| description_lines.append(first_line_value) | ||
|
|
||
| all_fields = ['Since', 'Catalog', 'Labels', 'Jira', 'History', 'Description'] |
There was a problem hiding this comment.
The list of all docstring fields is hardcoded here and in other places in this function (lines 137, 171). This makes the code harder to maintain. It would be better to define this list once as a constant at the top of the function, e.g., ALL_FIELDS = ['Since', 'Catalog', 'Labels', 'Jira', 'History', 'Description'], and reuse it throughout the function.
| if file_path.endswith('.sh'): | ||
| if not validate_shell_test_file(file_path): | ||
| sys.exit(1) | ||
| elif file_path.endswith('.py'): | ||
| if not validate_test_file(file_path): | ||
| sys.exit(1) | ||
| else: | ||
| print(f"Unsupported file type: {file_path}. Only .py and .sh files are supported.") | ||
| sys.exit(1) |
There was a problem hiding this comment.
The logic for selecting a validator based on the file extension and then calling it is duplicated. This can be refactored to be more DRY (Don't Repeat Yourself), which will make the code cleaner and easier to extend with more file types in the future.
| if file_path.endswith('.sh'): | |
| if not validate_shell_test_file(file_path): | |
| sys.exit(1) | |
| elif file_path.endswith('.py'): | |
| if not validate_test_file(file_path): | |
| sys.exit(1) | |
| else: | |
| print(f"Unsupported file type: {file_path}. Only .py and .sh files are supported.") | |
| sys.exit(1) | |
| validator = None | |
| if file_path.endswith('.sh'): | |
| validator = validate_shell_test_file | |
| elif file_path.endswith('.py'): | |
| validator = validate_test_file | |
| if validator: | |
| if not validator(file_path): | |
| sys.exit(1) | |
| else: | |
| print(f"Unsupported file type: {file_path}. Only .py and .sh files are supported.") | |
| sys.exit(1) |
Description
Please briefly describe the code changes in this pull request.
Checklist
Please check the items in the checklist if applicable.