scripts: skip processing lines in the allowlist section in merge_by_t… #21782
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
the
missing-translation-status
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @shhdgit, 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 enhances the merge_by_toc.py script by introducing a mechanism to selectively ignore specific sections of a document. The change allows the script to identify and skip processing content within a designated 'allowlist' section, improving the accuracy and flexibility of documentation generation by preventing unintended processing of certain text blocks.
Highlights
- New State Flag: Introduced a new boolean variable,
in_allowlist, to track whether the script is currently processing content within an allowlist section. - Allowlist Section Detection: Added logic to identify the beginning of an allowlist section, marked by
## _BUILD_ALLOWLIST, and its end, which is signaled by the next heading (#). - Skipping Processing: Implemented a conditional skip mechanism that prevents lines within the identified allowlist section from being processed by the script, ensuring they are ignored for TOC generation.
Using Gemini Code Assist
The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
ti-chi-bot
bot
added
the
size/XS
There was a problem hiding this comment.
Summary of Changes
Hello @shhdgit, 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 enhances the merge_by_toc.py script by introducing a mechanism to selectively ignore specific sections of content. The script will now identify and skip processing lines within an 'allowlist' block, ensuring that certain predefined content is not inadvertently included or altered during the table of contents merging process. This change improves the script's robustness and flexibility for handling varied documentation structures.
Highlights
- New State Variable: Introduced a new boolean flag,
in_allowlist, to track whether the script is currently parsing content within a designated allowlist section. - Allowlist Section Detection: Added logic to detect the beginning of an allowlist section, specifically when encountering a line that exactly matches
## _BUILD_ALLOWLIST. - Allowlist Section Termination: Implemented a mechanism to mark the end of an allowlist section, which occurs when a new heading (any line starting with
#) is encountered while the script is currentlyin_allowlist. - Skipping Processed Lines: Modified the main line processing loop to conditionally skip any lines that are found within an active allowlist section, preventing them from being processed by the script's regular logic.
Using Gemini Code Assist
The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism in the merge_by_toc.py script to skip processing lines within a special _BUILD_ALLOWLIST section in the TOC.md file. The implementation correctly identifies the start and end of this section using a state flag and skips the content within it. The logic is sound. I have one suggestion to refactor a conditional check to improve code readability. Otherwise, the changes are good.
| elif in_toc and not line.startswith("#") and line.strip(): | ||
| # Skip processing if we're in the allowlist section | ||
| if in_allowlist: | ||
| continue |
There was a problem hiding this comment.
For better readability, you can incorporate the nested if in_allowlist: check directly into the elif condition. This makes the condition for processing a TOC entry more explicit and self-contained, which aligns with the PEP 8 principle of readability.1
| elif in_toc and not line.startswith("#") and line.strip(): | |
| # Skip processing if we're in the allowlist section | |
| if in_allowlist: | |
| continue | |
| elif in_toc and not in_allowlist and not line.startswith("#") and line.strip(): |
Style Guide References
Footnotes
-
PEP 8, the style guide for Python code, emphasizes readability. Refactoring complex conditional logic to be clearer and more direct is a good practice that makes the code easier to understand and maintain. ↩
There was a problem hiding this comment.
Code Review
This pull request adds functionality to the merge_by_toc.py script to skip processing lines within a special ## _BUILD_ALLOWLIST section in the input Table of Contents file. This is achieved by introducing a new state flag, in_allowlist. While reviewing the implementation, I identified a logic bug in the order of conditional statements that could lead to the allowlist section being incorrectly processed as regular content. I have provided a specific comment with a code suggestion to rectify this issue.
| if not in_toc and not line.startswith("<!-- "): | ||
| in_toc = True | ||
| elif line.strip() == "## _BUILD_ALLOWLIST": | ||
| in_allowlist = True | ||
| elif in_allowlist and line.startswith("#"): | ||
| in_allowlist = False | ||
| elif in_toc and not line.startswith("#") and line.strip(): | ||
| # Skip processing if we're in the allowlist section | ||
| if in_allowlist: | ||
| continue |
There was a problem hiding this comment.
The current order of conditions in this if/elif chain can lead to incorrect behavior. If the ## _BUILD_ALLOWLIST marker appears in the input file before any other content that would set in_toc = True (for example, after the initial comments but before any list items), the first condition if not in_toc and not line.startswith("<!-- ") will be triggered by the ## _BUILD_ALLOWLIST line itself. This sets in_toc = True but fails to set in_allowlist = True, causing the script to treat the allowlist section as regular content.
To ensure the allowlist is always correctly identified, the checks for the _BUILD_ALLOWLIST section should be prioritized and moved before the logic that determines the start of the Table of Contents.
| if not in_toc and not line.startswith("<!-- "): | |
| in_toc = True | |
| elif line.strip() == "## _BUILD_ALLOWLIST": | |
| in_allowlist = True | |
| elif in_allowlist and line.startswith("#"): | |
| in_allowlist = False | |
| elif in_toc and not line.startswith("#") and line.strip(): | |
| # Skip processing if we're in the allowlist section | |
| if in_allowlist: | |
| continue | |
| if line.strip() == "## _BUILD_ALLOWLIST": | |
| in_allowlist = True | |
| elif in_allowlist and line.startswith("#"): | |
| in_allowlist = False | |
| elif not in_toc and not line.startswith("<!-- "): | |
| in_toc = True | |
| elif in_toc and not line.startswith("#") and line.strip(): | |
| # Skip processing if we're in the allowlist section | |
| if in_allowlist: | |
| continue |
…oc.py
First-time contributors' checklist
What is changed, added or deleted? (Required)
Which TiDB version(s) do your changes apply to? (Required)
Tips for choosing the affected version(s):
By default, CHOOSE MASTER ONLY so your changes will be applied to the next TiDB major or minor releases. If your PR involves a product feature behavior change or a compatibility change, CHOOSE THE AFFECTED RELEASE BRANCH(ES) AND MASTER.
For details, see tips for choosing the affected versions.
What is the related PR or file link(s)?
Do your changes match any of the following descriptions?