20233: Refactor example metadata parsing utilities(#20204)#233
20233: Refactor example metadata parsing utilities(#20204)#233martin-augment wants to merge 1 commit intomainfrom
Conversation
WalkthroughA monolithic documentation generation module at ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @martin-augment, 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 refactors the example metadata parsing utilities within the DataFusion examples project. The original monolithic 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
|
Pull Request Review: Refactor Example Metadata Parsing UtilitiesSummaryThis PR refactors Code Quality: ✅ ExcellentStrengths
Best Practices: ✅ GoodPositives
Minor Observations
Potential Issues:
|
![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 refactors the example metadata parsing and documentation generation logic into a new, well-structured example_metadata module. The code is now more modular and easier to maintain. I've found a couple of areas for improvement: a circular dependency between modules that should be resolved, and a fragile parsing implementation that could be made more robust. My detailed comments provide suggestions for addressing these points.
|
|
||
| use datafusion::error::{DataFusionError, Result}; | ||
|
|
||
| use crate::utils::example_metadata::{parse_main_rs_docs, render::ABBREVIATIONS}; |
There was a problem hiding this comment.
This use statement introduces a circular dependency between the model and render modules (model -> render for ABBREVIATIONS, and render -> model for ExampleGroup). This can make the code harder to understand and maintain.
Since ABBREVIATIONS is used for constructing the GroupName title, it's more of a model concern than a rendering one in the current structure. To break the cycle, I suggest moving the ABBREVIATIONS constant from render.rs to this file (model.rs) and updating this use statement accordingly.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! The ABBREVIATIONS constant is used only in model.rs, so there is no real need to keep it in render.rs and create a circular dependency between these two modules.
| fn parse_metadata_line(input: &str) -> IResult<&str, (&str, &str)> { | ||
| let parser = preceded( | ||
| multispace0, | ||
| preceded(tag("//!"), preceded(multispace0, take_while(|_| true))), | ||
| ); | ||
| let (rest, payload) = all_consuming(parser).parse(input)?; | ||
|
|
||
| let content = payload | ||
| .strip_prefix("(") | ||
| .and_then(|s| s.strip_suffix(")")) | ||
| .ok_or_else(|| { | ||
| nom::Err::Error(nom::error::Error::new(payload, nom::error::ErrorKind::Tag)) | ||
| })?; | ||
|
|
||
| let (file, desc) = content | ||
| .strip_prefix("file:") | ||
| .ok_or_else(|| { | ||
| nom::Err::Error(nom::error::Error::new(payload, nom::error::ErrorKind::Tag)) | ||
| })? | ||
| .split_once(", desc:") | ||
| .ok_or_else(|| { | ||
| nom::Err::Error(nom::error::Error::new(payload, nom::error::ErrorKind::Tag)) | ||
| })?; | ||
|
|
||
| Ok((rest, (file.trim(), desc.trim()))) | ||
| } |
There was a problem hiding this comment.
The use of split_once here is a bit fragile. It might incorrectly parse if the file path contains a comma, or if the description contains the string , desc:. A more robust approach would be to use nom combinators to parse the content within the parentheses. This will require updating the nom imports at the top of the file to include rest, separated_pair, and tuple.
fn parse_metadata_line(input: &str) -> IResult<&str, (&str, &str)> {
let parser = preceded(
multispace0,
preceded(tag("//!"), preceded(multispace0, take_while(|_| true))),
);
let (rest, payload) = all_consuming(parser).parse(input)?;
let content = payload
.strip_prefix("(")
.and_then(|s| s.strip_suffix(")"))
.ok_or_else(|| {
nom::Err::Error(nom::error::Error::new(payload, nom::error::ErrorKind::Tag))
})?;
let file_parser = preceded(nom::sequence::tuple((tag("file:"), multispace0)), take_until(", desc:"));
let desc_parser = preceded(nom::sequence::tuple((tag(", desc:"), multispace0)), nom::combinator::rest);
let (file, desc) = match nom::sequence::separated_pair(file_parser, multispace0, desc_parser)(content) {
Ok( K("", (file, desc))) => (file, desc),
_ => {
return Err(nom::Err::Error(nom::error::Error::new(
payload,
nom::error::ErrorKind::Tag,
)))
}
};
Ok((rest, (file.trim(), desc.trim())))
}| pub const ABBREVIATIONS: &[(&str, &str)] = &[ | ||
| ("dataframe", "DataFrame"), | ||
| ("io", "IO"), | ||
| ("sql", "SQL"), | ||
| ("udf", "UDF"), | ||
| ]; |
There was a problem hiding this comment.
To break a circular dependency with the model module, this ABBREVIATIONS constant should be moved to model.rs. The model module currently depends on render for this constant (to construct GroupName instances), while render depends on model for ExampleGroup.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! The ABBREVIATIONS constant is used only in model.rs, so there is no real need to keep it in render.rs and create a circular dependency between these two modules.
🤖 Augment PR SummarySummary: This PR refactors the example README generation utilities by splitting the former monolithic Changes:
Technical Notes: The refactor centralizes example-doc generation behind 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| let entry = entry?; | ||
| let path = entry.path(); | ||
|
|
||
| if path.is_dir() && path.join("main.rs").exists() { |
There was a problem hiding this comment.
path.join("main.rs").exists() will also return true if main.rs exists but isn’t a regular file (e.g., a directory or unexpected symlink). Consider checking is_file() (or metadata) so discovery only treats real main.rs files as valid example groups.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:useful; category:bug; feedback: The Augment AI reviewer is correct! An additional check that main.rs is a file should be added to prevent accepting main.rs folders as a group indicator.
| continue; | ||
| } | ||
|
|
||
| // If a non-blank doc line interrupts a pending subcommand, reset the state |
There was a problem hiding this comment.
parse_main_rs_docs only resets ParserState::SeenSubcommand when it encounters an unrelated non-blank doc line, so metadata could still be accepted after intervening non-doc lines (comments/code). If the intent is “metadata must immediately follow the subcommand (allowing only blank doc lines)”, consider also resetting state on other non-empty non-metadata lines.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Augment AI reviewer is correct! The whole state should be reset after seeing new non-empty content after an empty line. This indicates a new subcommand with its own metadata, so the state must be cleared.
value:good-to-have; category:bug; feedback: The Claude AI reviewer is correct! The ABBREVIATIONS constant is used only in model.rs, so there is no real need to keep it in render.rs and create a circular dependency between these two modules. |
value:valid-but-wont-fix; category:bug; feedback: The Claude AI reviewer is not correct! The file name and the line number is enough information to debug any potential problems. Printing the line itself might be helpful if the text is short but it will add noise for longer lines. |
2 participants
20233: To review by AI