Skip to content

Conversation

@DisturbedOcean
Copy link

I got tired of repeating myself in extremely large structs. So this adds support for re-using serde's rename / rename_all with precedence.

It also adds support for arrow_field at the struct/ container level.

Claude Assisted for transparency.

@gooroo-dev
Copy link

gooroo-dev bot commented Dec 25, 2025

Please double check the following review of the pull request:

🐞Mistake 🤪Typo 🚨Security 🚀Performance 💪Best Practices 📖Readability ❓Others
0 0 0 0 0 1 0

Changes in the diff

  • ➕ Added support for serde(rename_all) and serde(rename) attributes in arrow_convert derive macros.
  • ➕ Added precedence rules for renaming: #[arrow_field(name)] > #[serde(rename)] > rename_all > Rust field name.
  • ➕ Added RenameRule enum and implementation to parse and apply serde-style rename_all case conversions.
  • ➕ Added parsing of serde(rename_all) and serde(rename) attributes in container and field attribute parsing.
  • ➕ Updated derive logic to use effective names considering the new precedence rules.
  • ➕ Added extensive tests covering various rename_all cases, precedence, enum variant renaming, and round-trip serialization.
  • 📖 Improved README documentation to explain the new renaming features and precedence.
  • 🛠️ Added serde as an allowed attribute in derive macros.
  • 📖 Added comments and tests in case.rs for rename rules and splitting logic.

Identified Issues

ID Type Details Severity Confidence
1 📖Readability The split_into_words function in case.rs is complex and could be simplified or better documented for maintainability. 🟡 Low 🟡 Low

Issue 1

Explanation

In arrow_convert_derive/src/case.rs, the function split_into_words implements splitting a string into words based on underscores, hyphens, and case transitions, including handling acronyms. While it works correctly (as verified by tests), the logic is somewhat complex and could be hard to maintain or extend.

Suggested fix

Add more detailed comments explaining the logic step-by-step or consider simplifying the logic if possible. However, since this is a utility function with tests, this is a low priority.

/// Split a name into words based on underscores, hyphens, and case transitions.
/// Handles acronyms by grouping consecutive uppercase letters.
/// Examples:
/// - "myFieldName" -> ["my", "Field", "Name"]
/// - "XMLParser" -> ["XML", "Parser"]
/// - "parseXML" -> ["parse", "XML"]
fn split_into_words(name: &str) -> Vec<String> {
    // ... existing implementation ...
}

Explanation of fix

Adding detailed comments improves readability and maintainability without changing behavior.

Missing tests

The diff already includes extensive tests covering:

  • Field renaming with serde rename and rename_all.
  • Precedence between arrow_field and serde attributes.
  • Enum variant renaming.
  • Round-trip serialization/deserialization with renames.
  • Compound serde attributes combined with rename.

No additional tests are necessary for the introduced features.

Summary

The PR is well-implemented, adding a useful feature to support serde renaming conventions with proper precedence and extensive tests. The only minor suggestion is to improve readability of the word splitting function with more comments.

Summon me to re-review when updated! Yours, Gooroo.dev
Please add a reaction or reply to share your feedback.

@Swoorup
Copy link
Owner

Swoorup commented Jan 26, 2026

Sorry for the delay, just back from holidays. Catching things, up. This is great, but I don't see the need for supporting the attribute serde. Shouldn't arrow_field suffice? I don't want to conflict with serde if someone actually uses serde derive macros here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants