514: chore: Apply some suggestions by -Dclippy::pedantic

[martin-augment]

514: To review by AI

[coderabbitai]

Walkthrough

This pull request encompasses refactoring and enhancement across multiple components. The Rust CI workflow is modified to add workspace-scoped Clippy checks with specific lint configurations. Multiple files in avro_derive are updated to use borrowed references instead of owned values in function signatures, reducing unnecessary moves. Test utilities are expanded with public helper functions for logging assertions and message clearing. Enum schema generation and serialization code undergo minor control-flow improvements. Additionally, the test helper library exports new modules, adds #[inline] and const attributes, and updates type imports from std to core variants.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch pr-514-2026-03-24-14-31-02

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! 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 focuses on applying a series of code quality and style improvements, primarily driven by suggestions from clippy::pedantic. The changes span across multiple crates, enhancing code readability, correctness, and efficiency through refactoring function signatures to use references, improving iteration patterns, and adding comprehensive documentation to test utilities.

Highlights

• Code Refactoring for Efficiency: Several functions across avro_derive were updated to accept arguments by reference (&T) instead of by value (T), reducing unnecessary cloning and improving performance.
• Clippy Pedantic Suggestions Applied: Various minor code style and correctness improvements were made, such as adding missing semicolons, refactoring if conditions into match guards, and using zip for cleaner iteration, aligning with clippy::pedantic suggestions.
• Improved Test Helper Utilities: The avro_test_helper crate received updates including better documentation for logging functions, #[inline] attributes for performance, and minor code cleanup.
• Enhanced Error Handling: The to_compile_errors function in avro_derive was updated to accept a slice of errors, making error reporting more flexible.

Ignored Files

• Ignored by pattern: .github/workflows/** (1)
  • .github/workflows/test-lang-rust-clippy.yml

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[claude]

PR Review: Apply clippy::pedantic suggestions

Overall this is a solid, mechanical improvement PR that makes the codebase more idiomatic Rust. Most changes are straightforward clippy pedantic fixes. A few items worth discussing:

---

Bug: Unclosed doc comment code block in assert_not_logged

File: avro_test_helper/src/logger.rs

The /// ```ignore block in the assert_not_logged docstring is never closed with a closing ```. This will trigger a rustdoc warning about an unclosed code block example.

/// # Example
/// ```ignore
/// // Assume LOG_MESSAGES is set up to capture log messages.
/// log_message("Unexpected Error");
/// assert_not_logged("Unexpected Error");
/// // This will panic with the message:
/// // "The following log message should not have been logged: 'Unexpected Error'"
///
/// assert_not_logged("Non-existent Message");
/// // This will pass without issue since the message was not logged.
// ← missing: /// ```
#[track_caller]
pub fn assert_not_logged(unexpected_message: &str) {

The assert_logged example is properly closed, so this appears to be an oversight.

---

Minor: Option<&String> could be Option<&str>

File: avro_derive/src/attributes/mod.rs

// Changed from:
fn from_avro_and_serde(avro: &avro::With, serde: &Option<String>, ...) -> ...
// To:
fn from_avro_and_serde(avro: &avro::With, serde: Option<&String>, ...) -> ...

The change from &Option<String> to Option<&String> is correct and more ergonomic, but clippy pedantic would typically also prefer Option<&str> over Option<&String> (the clippy::option_as_ref_deref / clippy::ptr_arg family of lints). This could be taken one step further for maximum idiomaticity.

---

Observation: Verbose docstrings on internal test helpers

File: avro_test_helper/src/logger.rs

The added docstrings for assert_logged and assert_not_logged are significantly more verbose than the functions themselves. For instance, assert_logged's doc comment spans ~35 lines for a ~10-line function. For internal test helpers, a brief 1-2 line description is more maintainable. The "Thread Safety" and "Notes" sections especially feel like they document implementation details that should be evident from the code.

---

Positive highlights

• case.rs: Merging UpperCase | ScreamingSnakeCase into a single match arm is correct — both cases call field.to_ascii_uppercase() — and eliminates duplication cleanly.
• lib.rs: Changing to_compile_errors to accept &[syn::Error] instead of Vec<syn::Error> is a better API design (avoids unnecessary ownership transfer).
• logger.rs: Using assert!(deleted, ...) instead of if !deleted { panic!(...) } and .expect("...") instead of .unwrap() are clear improvements.
• append_to_existing.rs: The (1..).zip(reader) pattern is more idiomatic than a manual counter.
• CI: Adding --workspace to the clippy command ensures all workspace members are linted, not just the default target.
• #[non_exhaustive] on TestError: Good forward-compatibility practice.

---

Summary

One actual bug (unclosed doc comment block) and two minor style suggestions. The bulk of the changes are correct and improve code quality.

[augmentcode]

🤖 Augment PR Summary

Summary: This PR applies a batch of clippy (incl. pedantic/cargo) cleanups and small refactors across the workspace.

Changes:

• Adjust CI clippy invocation to lint the whole workspace and enable stricter clippy groups.
• Simplify/clean up match arms, iterator usage, and imports in avro and avro_derive.
• Refactor derive-macro helpers to pass references instead of moving owned values.
• Tighten test-helper utilities (logger assertions, init API) and expand rustdoc for logging helpers.

Technical Notes: Most changes are behavior-preserving and aim to satisfy stricter lint settings while reducing unnecessary moves/clones.

_{🤖 Was this summary useful? React with 👍 or 👎}

[augmentcode]

Review completed. 2 suggestions posted.

Comment augment review to trigger a new review at any time.

[augmentcode]

The rustdoc example for assert_not_logged starts a fenced code block (```ignore) but doesn’t close it; this can break rustdoc rendering (and potentially doc builds) for the rest of the item/file.

Severity: high

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

[martin-augment]

value:useful; category:bug; feedback: The Augment AI reviewer is correct! The code snippet in the docstring is not closed and this breaks the doc tests. Prevents CI failures.

[augmentcode]

Adding #[non_exhaustive] to the public TestError changes its public API (e.g., external users can no longer construct it), which can be a breaking change if avro_test_helper is consumed outside this workspace.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

[martin-augment]

value:good-but-wont-fix; category:bug; feedback: The Augment AI reviewer is correct! The external users are not supposed to construct this error. It is a helper error that just unifies any other kind of errors, so that they could be used with TestResult in avro-rs' unit/IT tests.

[gemini-code-assist]

Code Review

This pull request applies a number of stylistic and idiomatic improvements suggested by clippy::pedantic. The changes are generally good, improving code clarity and following Rust best practices. I've found one area in the test helper logging utilities where the documentation is ambiguous and could lead to confusion in tests, and I've provided a suggestion to clarify it.

[gemini-code-assist]

The documentation states that if the message is found, 'it is removed from the log messages'. This is ambiguous when the same message is logged multiple times. The current implementation uses retain, which removes all occurrences of the message. This can lead to unexpected behavior in tests.

For example, if a test does this:

log("some message");
log("some message");
assert_logged("some message"); // This will pass, but removes BOTH "message" logs.
assert_logged("some message"); // This will now FAIL, which is likely not the intended behavior.

A more correct implementation for a test helper like this would be to remove only the first occurrence of the message. This would allow for asserting multiple identical logs.

Consider changing the implementation to something like this:

    let mut deleted = false;
    LOG_MESSAGES.with(|msgs| {
        if let Some(pos) = msgs.borrow().iter().position(|msg| msg == expected_message) {
            msgs.borrow_mut().remove(pos);
            deleted = true;
        }
    });

Alternatively, if removing all occurrences is the desired behavior, the documentation should be updated to reflect that, for example: 'all occurrences of the message are removed'.

[martin-augment]

value:useful; category:bug; feedback: The Gemini AI reviewer is correct! The assert_logged() test helper method should delete only one matching message, not all of them. The helper method should be called the same number of times as the message has been logged to remove all of them.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

avro_test_helper/src/logger.rs (1)

71-105: ⚠️ Potential issue | 🟠 Major

assert_not_logged only checks the last message, which may cause false negatives.

The function uses msgs.borrow().last() to check only the most recently logged message. Based on the existing test pattern in avro/src/types.rs (lines 1370-1450), assert_not_logged is called after multiple validation failures that each log a message. If an earlier message in the sequence matches unexpected_message but the last message is different, this assertion will incorrectly pass.

Consider checking all messages instead:

🐛 Proposed fix to check all messages

 #[track_caller]
 pub fn assert_not_logged(unexpected_message: &str) {
-    LOG_MESSAGES.with(|msgs| match msgs.borrow().last() {
-        Some(last_log) if last_log == unexpected_message => {
+    LOG_MESSAGES.with(|msgs| {
+        if msgs.borrow().iter().any(|msg| msg == unexpected_message) {
             panic!("The following log message should not have been logged: '{unexpected_message}'")
         }
-        _ => (),
     });
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@avro_test_helper/src/logger.rs` around lines 71 - 105, The assert_not_logged
function only compares unexpected_message to the most recent entry via
LOG_MESSAGES.with(|msgs| msgs.borrow().last()), which misses earlier matching
logs; update assert_not_logged to iterate over all stored messages in
LOG_MESSAGES (e.g., use msgs.borrow().iter().any(...) or equivalent) and panic
if any entry equals unexpected_message so it correctly detects matches anywhere
in the captured logs (refer to LOG_MESSAGES and assert_not_logged to locate the
change).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@avro_test_helper/src/logger.rs`:
- Around line 71-105: The assert_not_logged function only compares
unexpected_message to the most recent entry via LOG_MESSAGES.with(|msgs|
msgs.borrow().last()), which misses earlier matching logs; update
assert_not_logged to iterate over all stored messages in LOG_MESSAGES (e.g., use
msgs.borrow().iter().any(...) or equivalent) and panic if any entry equals
unexpected_message so it correctly detects matches anywhere in the captured logs
(refer to LOG_MESSAGES and assert_not_logged to locate the change).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: cd07b9f5-c3b2-4b7d-b762-6443409c3140

📥 Commits

Reviewing files that changed from the base of the PR and between 7648438 and 1bf28f7.

📒 Files selected for processing (13)

• .github/workflows/test-lang-rust-clippy.yml
• avro/src/serde/ser_schema/mod.rs
• avro/tests/append_to_existing.rs
• avro_derive/build.rs
• avro_derive/src/attributes/avro.rs
• avro_derive/src/attributes/mod.rs
• avro_derive/src/case.rs
• avro_derive/src/enums/mod.rs
• avro_derive/src/enums/plain.rs
• avro_derive/src/lib.rs
• avro_test_helper/src/data.rs
• avro_test_helper/src/lib.rs
• avro_test_helper/src/logger.rs

[martin-augment]

Bug: Unclosed doc comment code block in assert_not_logged

File: avro_test_helper/src/logger.rs

The /// ```ignore block in the assert_not_logged docstring is never closed with a closing ```. This will trigger a rustdoc warning about an unclosed code block example.

/// # Example
/// ```ignore
/// // Assume LOG_MESSAGES is set up to capture log messages.
/// log_message("Unexpected Error");
/// assert_not_logged("Unexpected Error");
/// // This will panic with the message:
/// // "The following log message should not have been logged: 'Unexpected Error'"
///
/// assert_not_logged("Non-existent Message");
/// // This will pass without issue since the message was not logged.
// ← missing: /// ```
#[track_caller]
pub fn assert_not_logged(unexpected_message: &str) {

The assert_logged example is properly closed, so this appears to be an oversight.

value:useful; category:bug; feedback: The Claude AI reviewer is correct! The code snippet in the docstring is not closed and this breaks the doc tests. Prevents CI failures.

[martin-augment]

Minor: Option<&String> could be Option<&str>

File: avro_derive/src/attributes/mod.rs

// Changed from:
fn from_avro_and_serde(avro: &avro::With, serde: &Option<String>, ...) -> ...
// To:
fn from_avro_and_serde(avro: &avro::With, serde: Option<&String>, ...) -> ...

The change from &Option<String> to Option<&String> is correct and more ergonomic, but clippy pedantic would typically also prefer Option<&str> over Option<&String> (the clippy::option_as_ref_deref / clippy::ptr_arg family of lints). This could be taken one step further for maximum idiomaticity.

value:valid-but-wont-fix; category:bug; feedback: The Claude AI reviewer is not correct! Usually this is a good suggestion but in this particular case this is not possible because Option<String> comes from a struct field and this struct does not use lifetimes to be able to use a str slice. Making this change will lead to many other unrelated changes and complexities related to lifetimes.

[martin-augment]

71-105: ⚠️ Potential issue | 🟠 Major

assert_not_logged only checks the last message, which may cause false negatives.

The function uses msgs.borrow().last() to check only the most recently logged message. Based on the existing test pattern in avro/src/types.rs (lines 1370-1450), assert_not_logged is called after multiple validation failures that each log a message. If an earlier message in the sequence matches unexpected_message but the last message is different, this assertion will incorrectly pass.

value:useful; category:bug; feedback: The CodeRabbit AI reviewer is correct! The assert_not_logged() helper method should check in all messages, not just the last one. This will make it more usable in the unit/IT tests.