472: feat!: Use bon builder style instead of several methods#52
472: feat!: Use bon builder style instead of several methods#52martin-augment wants to merge 6 commits intomainfrom
Conversation
Instead of having several methods like: Schema::array(Schema) and Schema::array_with_attributes(Schema, BTreeMap) we could use Bon's function builders with start_fn: ``` Schema::array(Schema).call() Schema::array(Schema).attributes(BTreeMap).call() ``` The only annoying part is the `.call()`
|
No actionable comments were generated in the recent review. 🎉 WalkthroughThis pull request refactors the Reader and Schema construction APIs to use builder patterns enabled by the ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 significantly refactors the API for creating 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 is a significant and well-executed refactoring that introduces the builder pattern for creating Reader and Schema objects, using the bon crate. This change improves the API by making object instantiation more explicit and flexible, especially for objects with multiple optional parameters. The updates have been applied consistently across the entire codebase, including documentation, examples, and tests. I have one minor suggestion to further improve code conciseness.
| let schemata = match schemata { | ||
| Some(schemata) => schemata, | ||
| None => match schema { | ||
| Some(schema) => vec![schema], | ||
| None => vec![], | ||
| }, | ||
| }; |
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! The logic for determining the schemata could be simplified to a one-liner. This will make it easier to understand and to maintain.
Code ReviewThis PR introduces the Issues1. Semantic bug risk:
|
🤖 Augment PR SummarySummary: This PR updates the public API to use Changes:
Technical Notes: The new APIs allow optional reader schema and optional “schemata” inputs for resolving schema references, while making schema attributes configuration explicit via builder chaining. 🤖 Was this summary useful? React with 👍 or 👎 |
value:valid-but-wont-fix; category:bug; feedback: The Claude AI reviewer is correct! The new builder API indeed allows to construct a Reader without a reader schema in contrast to the old API. The |
value:good-to-have; category:documentation; feedback: The Claude AI reviewer is correct! The |
value:good-but-wont-fix; category:bug; feedback: The Claude AI reviewer is correct! The old API was shorter in the minimal case where only the Schema is provided but longer when the custom attributes and the default field were also provided. The new builder style API also allows adding more fields in the future without API breaks. |
value:valid-but-wont-fix; category:bug; feedback: The Claude AI reviewer is correct! The new builder API indeed allows to construct a Reader without a reader schema in contrast to the old API. The schemata registry is needed only to resolve references in the reader schema, so it does not make much sense to set the schemata without a reader schema, but it also does not do any harm. Setting only the reader schema without schemata is totally OK |
value:annoying; category:bug; feedback: The Claude AI reviewer is not correct! The "bon" dependency was there already and it was used in few other places. |
472: To review by AI