feat: add support for tagged documents with transparent rendering#12
feat: add support for tagged documents with transparent rendering#12Enter-tainer wants to merge 1 commit intomainfrom
Conversation
Summary of ChangesHello @Enter-tainer, 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 introduces a powerful new feature that allows associating arbitrary Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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
The pull request introduces support for tagged documents, which is a great feature for enabling more advanced tooling on top of the pretty-printer. The implementation is well-structured, with changes to the core Doc enum, the rendering engine, and the addition of helper methods. The inclusion of tests for the new functionality is also very good. I've found one bug in the new debug_with_tag_ranges utility function that could lead to incorrect results in some cases. I've also suggested a small refactoring in the rendering logic to improve readability. Overall, this is a solid contribution.
| pub fn debug_with_tag_ranges<'a, T>(doc: &Doc<'a, T>) -> (String, Vec<DebugTagRange>) | ||
| where | ||
| T: DocPtr<'a> + fmt::Debug, | ||
| { | ||
| let text = format!("{doc:#?}"); | ||
| let mut tagged_tokens = Vec::new(); | ||
| collect_tagged_tokens(doc, None, &mut tagged_tokens); | ||
|
|
||
| let mut cursor = 0; | ||
| let mut ranges = Vec::new(); | ||
| for (tag, token) in tagged_tokens { | ||
| if token.is_empty() { | ||
| continue; | ||
| } | ||
| if let Some(found) = text[cursor..].find(&token) { | ||
| let start = cursor + found; | ||
| let end = start + token.len(); | ||
| ranges.push(DebugTagRange { tag, start, end }); | ||
| cursor = end; | ||
| } | ||
| } | ||
|
|
||
| (text, ranges) | ||
| } |
There was a problem hiding this comment.
The logic in debug_with_tag_ranges for finding character ranges of tagged documents is not robust. It relies on a string search (find) within the complete debug output, which can produce incorrect ranges if the same text appears in both tagged and untagged contexts.
For example, for a document like arena.text("a") + arena.text("a").tag(1), the debug output will contain two instances of "a". The search for the tagged "a" might incorrectly match the first, untagged instance, leading to a wrong range.
To fix this, you could consider a more robust approach, such as having collect_tagged_tokens also account for untagged text segments to ensure the search is performed on the correct parts of the string, or by modifying the debug printing logic to directly output range information.
| let CmdKind::Doc(doc) = cmd.kind else { | ||
| let CmdKind::TagExit(id) = cmd.kind else { | ||
| unreachable!(); | ||
| }; | ||
| out.on_tag_exit(id)?; | ||
| break; | ||
| }; |
There was a problem hiding this comment.
The nested let-else construct here for handling CmdKind is functionally correct but can be a bit hard to follow. Using a match statement would make the intent clearer and is a more idiomatic way to handle different enum variants in Rust. This would improve code readability and maintainability.
| let CmdKind::Doc(doc) = cmd.kind else { | |
| let CmdKind::TagExit(id) = cmd.kind else { | |
| unreachable!(); | |
| }; | |
| out.on_tag_exit(id)?; | |
| break; | |
| }; | |
| let doc = match cmd.kind { | |
| CmdKind::Doc(doc) => doc, | |
| CmdKind::TagExit(id) => { | |
| out.on_tag_exit(id)?; | |
| break; | |
| } | |
| }; |
1 participant
No description provided.