Conversation
Replace duplicated manual Parse impls across attribute modules with a shared declarative macro. Extract common parsing patterns into helper functions (parse_required_string, parse_flag, parse_string_or_list). - Merge FieldArguments and TypeSchemaArguments into a single struct - Delete typeschema.rs (nullable absorbed into fields.rs) - All parsers now consistently error on unknown attributes - Net reduction of ~224 lines
📝 WalkthroughWalkthroughThis pull request refactors attribute parsing in ronky_derive by replacing multiple manual Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ronky_derive/src/parsers/attributes/enum_variants.rs (1)
36-41: Error span could be improved for invalid transform values.When
EnumTransformation::try_from(s)fails,input.error(e)creates an error with the span at the current stream position (after parsing the list), rather than at the specific invalid value. This may produce less precise error locations for users.This is a minor UX concern and acceptable for this refactor scope.
♻️ Optional: Preserve span from LitStr for better error location
To preserve spans, you could modify
parse_string_or_listto returnVec<LitStr>instead ofVec<String>, then use the span from the failing literal:"transform" => { let transforms = parse_string_or_list_lits(input, "transform")?; let transforms = transforms .into_iter() .map(|lit| { EnumTransformation::try_from(lit.value()) .map_err(|e| syn::Error::new(lit.span(), e)) }) .collect::<Result<Vec<_>, _>>()?; args.transform.extend(transforms); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ronky_derive/src/parsers/attributes/enum_variants.rs` around lines 36 - 41, The error span for invalid transform values is currently created via input.error(e) after parsing strings, which points to the stream position instead of the specific literal; change parsing to return literals so you can use the literal's span when mapping to EnumTransformation: update parse_string_or_list to a variant (e.g., parse_string_or_list_lits) that returns Vec<LitStr>, then in the transform arm map each lit to EnumTransformation::try_from(lit.value()).map_err(|e| syn::Error::new(lit.span(), e)) and collect into Vec<_> before extending args.transform, replacing use of input.error and parse_string_or_list.ronky_derive/src/parsers/fields/base.rs (1)
41-66: Lastis_nullablevalue wins when multiple attributes present.The loop iterates through all
field_attrswithout breaking after finding the firstis_nullable, meaning if a field has multiple#[arri(nullable = ...)]attributes, the last one takes precedence. This is likely fine in practice but could be surprising behavior.Consider adding an early
breakafter line 58 if you want first-wins semantics, or document that last-wins is intentional. Alternatively, you could error ifactual_nullableis alreadySometo prevent conflicting attributes.♻️ Optional: Error on duplicate nullable specification
for attr in &field_attrs { if let Some(is_nullable) = attr.is_nullable { + if actual_nullable.is_some() { + return Err(quote_spanned!(field.span() => + compile_error!("Multiple nullable specifications found for field") + ) + .into()); + } // Ensure only optional types can be nullable. if !is_optional && is_nullable {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ronky_derive/src/parsers/fields/base.rs` around lines 41 - 66, The code currently lets the last #[arri(nullable = ...)] attribute win because the loop over field_attrs keeps overwriting actual_nullable; change this to detect duplicates and error: inside the for attr in &field_attrs loop where you check if let Some(is_nullable) = attr.is_nullable, if actual_nullable.is_some() return a compile_error! (use quote_spanned! with field.ty.span() to produce a helpful message about duplicate nullable attributes), otherwise set actual_nullable = Some(is_nullable); this prevents conflicting nullable specifications and points the user to the field causing the issue.ronky_derive/src/parsers/attributes/mod.rs (1)
37-41: Add validation to reject empty transform lists or clarify if intended.
parse_string_or_listacceptstransform = []and returns an emptyVec, which becomes a no-op in code generation. The function's documentation and all test cases show non-empty lists only. Either add validation to reject empty lists at parse time, or document that empty lists are intentionally allowed as no-ops.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ronky_derive/src/parsers/attributes/mod.rs` around lines 37 - 41, The parser currently accepts an empty bracketed transform list (seen in parse_string_or_list where you parse with Punctuated::<LitStr, Token![,]>::parse_terminated after bracketed!(content in input)), which yields an empty Vec and becomes a no-op; update parse_string_or_list to validate and reject empty lists by returning a syn::Error (with a clear message like "transform list cannot be empty") when list.is_empty(), or alternatively update the function docs/tests to state that empty lists are permitted—preferably implement the validation to fail at parse time so consumers get immediate feedback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ronky_derive/src/parsers/attributes/enum_variants.rs`:
- Around line 36-41: The error span for invalid transform values is currently
created via input.error(e) after parsing strings, which points to the stream
position instead of the specific literal; change parsing to return literals so
you can use the literal's span when mapping to EnumTransformation: update
parse_string_or_list to a variant (e.g., parse_string_or_list_lits) that returns
Vec<LitStr>, then in the transform arm map each lit to
EnumTransformation::try_from(lit.value()).map_err(|e|
syn::Error::new(lit.span(), e)) and collect into Vec<_> before extending
args.transform, replacing use of input.error and parse_string_or_list.
In `@ronky_derive/src/parsers/attributes/mod.rs`:
- Around line 37-41: The parser currently accepts an empty bracketed transform
list (seen in parse_string_or_list where you parse with Punctuated::<LitStr,
Token![,]>::parse_terminated after bracketed!(content in input)), which yields
an empty Vec and becomes a no-op; update parse_string_or_list to validate and
reject empty lists by returning a syn::Error (with a clear message like
"transform list cannot be empty") when list.is_empty(), or alternatively update
the function docs/tests to state that empty lists are permitted—preferably
implement the validation to fail at parse time so consumers get immediate
feedback.
In `@ronky_derive/src/parsers/fields/base.rs`:
- Around line 41-66: The code currently lets the last #[arri(nullable = ...)]
attribute win because the loop over field_attrs keeps overwriting
actual_nullable; change this to detect duplicates and error: inside the for attr
in &field_attrs loop where you check if let Some(is_nullable) =
attr.is_nullable, if actual_nullable.is_some() return a compile_error! (use
quote_spanned! with field.ty.span() to produce a helpful message about duplicate
nullable attributes), otherwise set actual_nullable = Some(is_nullable); this
prevents conflicting nullable specifications and points the user to the field
causing the issue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c0aa378-a43a-4f00-b2bb-2bea2962ef16
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
arri_repr/Cargo.tomlronky_derive/src/parsers/attributes/enum_variants.rsronky_derive/src/parsers/attributes/fields.rsronky_derive/src/parsers/attributes/mod.rsronky_derive/src/parsers/attributes/properties.rsronky_derive/src/parsers/attributes/typeschema.rsronky_derive/src/parsers/fields/base.rs
💤 Files with no reviewable changes (2)
- arri_repr/Cargo.toml
- ronky_derive/src/parsers/attributes/typeschema.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #127
This doesn't change any external API
Summary by CodeRabbit
Refactor
Chores