Conversation
Summary of ChangesHello @CybotTM, 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 addresses two critical bugs in the Highlights
Changelog
Activity
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
|
There was a problem hiding this comment.
Code Review
This pull request effectively addresses two bugs in TextRoleRule related to escape sequence handling. The logic for resolving \\ in $rawPart and the fallback for escaped backticks at the end of a role are well-implemented. The new unit and functional tests provide good coverage for these fixes. I have one suggestion to improve code maintainability by refactoring duplicated logic.
...uides-restructured-text/src/RestructuredText/Parser/Productions/InlineRules/TextRoleRule.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Fixes escape-sequence handling for reStructuredText text roles (notably code-like roles such as :code:), ensuring escaped backslashes are represented correctly in raw role content and adding a recovery path when the lexer swallows a closing backtick via ```-style tokenization.
Changes:
- Adjust
TextRoleRuleto resolve escaped backslashes (\\→\) in$rawPart, while preserving other escapes verbatim for code contexts. - Add an EOF fallback in
TextRoleRuleto handle the lexer’s special ``` + closing-backtick tokenization case. - Extend unit + functional coverage with new cases and updated expected HTML outputs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
packages/guides-restructured-text/src/RestructuredText/Parser/Productions/InlineRules/TextRoleRule.php |
Fixes raw escape handling and adds EOF fallback logic to close roles when backticks are swallowed. |
packages/guides-restructured-text/tests/unit/Parser/Productions/InlineRules/TextRoleRuleTest.php |
Adds unit test cases for escaped backslash and escaped-backtick-at-end scenarios. |
tests/Functional/tests/code-textrole-no-escape/code-textrole-no-escape.html |
Updates expected output to reflect \\ → \ behavior in :code: roles. |
tests/Functional/tests/code-textrole-escape/code-textrole-escape.rst |
New functional input covering backslash + backtick edge cases in :code: roles. |
tests/Functional/tests/code-textrole-escape/code-textrole-escape.html |
New functional expected output validating the new escape behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...uides-restructured-text/src/RestructuredText/Parser/Productions/InlineRules/TextRoleRule.php
Outdated
Show resolved
Hide resolved
...uides-restructured-text/src/RestructuredText/Parser/Productions/InlineRules/TextRoleRule.php
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses two critical bugs related to escape sequence handling in TextRoleRule. The changes correctly resolve \\ to \ in $rawPart for code contexts and implement a robust fallback mechanism to handle cases where an escaped backtick (\``) incorrectly consumes the closing delimiter. The introduction of the createTextRoleNode` private method enhances code maintainability by encapsulating node creation logic. Furthermore, the addition of comprehensive unit and functional tests, along with the update of an existing functional test, demonstrates thorough validation of the fixes. The solution is well-implemented and directly resolves the reported issues, improving the robustness of text role parsing.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/gemini review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fix two bugs causing code-type text roles (:rst:, :php:, :code:, etc.) to mishandle escape sequences: 1. $rawPart preserved \\ instead of resolving to \. Now resolved for escaped-backslash; other escapes (\T, \*) stay raw for code contexts (e.g. PHP namespaces). 2. The lexer's \`` catchable pattern (3 chars) swallows the closing backtick as part of an ESCAPED_SIGN token. The role never closes and rolls back. Now detected post-loop: the escape is consumed and a literal backtick becomes content. Narrowed to only the 3-char token; regular 2-char escapes at EOF correctly roll back. Resolves: TYPO3-Documentation/render-guides#1188
6be902c to
de35afa
Compare
Summary
Fixes escape handling bugs in
TextRoleRulefor code-type text roles (:rst:,:php:,:code:, etc.).Resolves: TYPO3-Documentation/render-guides#1188
Bug 1: Escaped backslash stays literal in rawPart
Code-type text roles use
$rawContent(via phpDocumentor#533). When the author writes\\to display a single backslash,$rawPartpreserved the literal\\instead of resolving it.RST input:
Before (broken):
After (fixed):
Other escapes like
\Tor\*are intentionally preserved raw in$rawPart— code contexts need literal backslash-letter sequences (e.g., PHP namespaces like\App\Entity).Bug 2: Escaped backtick swallows closing delimiter
The lexer has a catchable pattern that tokenizes a 3-char sequence (backslash + two backticks) as a single
ESCAPED_SIGNtoken. This swallows the closing backtick of the text role, soTextRoleRulenever finds aBACKTICKtoken to close — it rolls back and the role breaks entirely.RST input:
Before (broken): role is not recognized, rendered as literal text:
After (fixed): post-loop recovery detects the swallowed backtick. The 3-char token is split semantically:
The escape character is consumed, the backtick becomes content:
The recovery is narrowed to only the 3-char token — the only lexer pattern that swallows backticks. Regular 2-char escapes at end-of-input (like
\Tor\\without a closing backtick) correctly roll back as genuinely unterminated roles:RST input (genuinely unterminated — no closing backtick):
Result: rolls back, no role node produced (correct — the role was never closed).
Changes
TextRoleRule.php:\\→\in$rawPart(other escapes preserved raw)createTextRoleNode()helper to deduplicate role-building logic$lastEscapedToken)TextRoleRuleTest.php:\Tand\\at end → assert rollbackFunctional tests:
code-textrole-no-escape.htmlexpected outputcode-textrole-escape/test fixtureTest plan