refactor: dont need ref and ref mut#580
Conversation
|
Last commit removes some unused error conversions. I don't know for what these conversions from |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors pattern matching across the codebase to remove the legacy ref and ref mut keywords in favor of modern Rust match ergonomics introduced in RFC2005. The changes improve code readability and align with contemporary Rust idioms.
Key changes:
- Removes
refandref mutfrom match patterns in XML event handling - Updates match expressions to leverage automatic dereferencing (e.g.,
match *self→match self) - Adjusts function calls to explicitly pass references where needed after pattern ownership changes
- Removes unused
from_err!macro forstd::string::ParseErrorand addsfrom_err!forstd::str::ParseBoolError
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/xlsx/mod.rs | Removed ref from XML event patterns, removed from_err! for ParseError, converted if let Some(ref mut s) to if let Some(s) = &mut |
| src/xlsx/cells_reader.rs | Removed ref from event patterns and adjusted function calls to pass explicit references |
| src/xlsb/mod.rs | Removed ref from XML event pattern matching |
| src/ods.rs | Removed ref from event patterns, replaced ParseError from_err! with ParseBool, simplified error conversions |
| src/de.rs | Changed match *self to match self and removed ref from field patterns |
| src/datatype.rs | Updated Display and PartialEq implementations to use match ergonomics |
| src/auto.rs | Removed ref and ref mut from enum variant patterns |
| examples/excel_to_csv.rs | Updated match expression to use automatic dereferencing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@jqnatividad Looked at the suggested fixes by copilot. While I agree that in its current form the |
|
@sftse Overall this looks fine to me. Could you please resolve/close the Copilot reviews with a comment about why each isn't relevant or required. Let me know when it is ready to merge. |
|
GTM |
|
Merged. Thanks. |
Not strictly necessary, but while reviewing #559 noticed a lot of
refandref mutuse which is a mostly superfluous mechanism to express whether the pattern should be borrowing or take ownership. Since RFC2005 landed it is a pattern that is harder to explain than using intuition and the&/&mutsigils.