feat(field_attributes): add helper functions #41
Conversation
WalkthroughThe update focuses on optimizing deserialization by refining trait implementations and debugging processes in the codebase. It involves removing Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (1)
- src/field_attributes.rs (29 hunks)
Additional comments: 12
src/field_attributes.rs (12)
- 286-292: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [264-317]
The function
deserialize_number_from_stringis well-implemented, leveraging Rust's type system and serde's capabilities to deserialize numbers from either string or numeric inputs. This function enhances the flexibility of input data handling, which is particularly useful in scenarios where the data source might not be consistent in its representation of numbers.
- 337-343: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [340-397]
The function
deserialize_option_number_from_stringcorrectly handles optional numeric fields, allowing for a graceful handling ofnull, empty strings, or valid numeric values (both as strings and as numbers). This function is a valuable addition for APIs or data formats where optional fields may be omitted or presented in different formats.
- 543-549: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [546-669]
The function
deserialize_bool_from_anythingintroduces a robust way to deserialize boolean values from various input formats, including strings, integers, floats, and booleans. This function significantly enhances flexibility and error tolerance when dealing with diverse data sources. However, it's important to ensure that this flexibility aligns with the expected data integrity and validation requirements of the application.
- 666-672: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [669-714]
The function
deserialize_string_from_numberprovides a straightforward and useful functionality to deserialize numeric values as strings, which can be particularly useful in contexts where numeric values need to be treated as text, such as identifiers or codes. This function complements the library's deserialization capabilities well.
- 711-717: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [714-747]
The function
deserialize_default_from_nullis a practical utility for handlingnullvalues by substituting them with default values of the specified type. This function can simplify handling optional fields in data structures, ensuring that they have sensible defaults when not explicitly provided.
- 747-753: The function
deserialize_default_from_empty_objectintroduces a nuanced approach to handlingnullor empty objects by defaulting to a specified type's default value. This function is particularly useful for complex nested structures where an empty or missing object should not lead to an error but rather a default state. It's a thoughtful addition to the library's deserialization utilities.- 802-808: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [805-821]
The function
deserialize_vec_from_string_or_vecis a versatile utility that allows for deserializing a list of elements either from a comma-separated string or a JSON array. This function enhances the library's flexibility in handling list inputs, accommodating various data representation formats seamlessly.
- 828-884: The function
deserialize_to_type_or_fallbackintroduces an innovative approach to deserialization by attempting to deserialize to a primary type and falling back to a secondary type if the first attempt fails. This function can be particularly useful in scenarios where data might be represented in multiple valid formats, and the application needs to handle all such formats gracefully.- 886-922: The function
deserialize_to_type_or_noneprovides a fail-safe deserialization mechanism where, instead of failing on incompatible types, it returnsNone. This approach is beneficial for optional fields where data integrity is not strictly enforced, and the absence of a value is an acceptable outcome.- 924-987: The function
deserialize_to_type_or_string_lossyoffers a flexible deserialization strategy that attempts to deserialize to a specified type and, upon failure, captures the input as a string. This function is particularly useful for logging or error reporting purposes, where understanding the original input can provide valuable context for debugging.- 994-1000: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [997-1014]
The macro
StringOrVecToVecParseris a powerful tool for creating custom deserializers that can handle both string and vector inputs, converting them into a vector of a specified type. This macro significantly enhances the library's deserialization capabilities, allowing for custom parsing logic that can accommodate complex input scenarios.
- 1116-1122: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1071-1169]
The
StringOrVecToVecstruct and its associated methods provide a comprehensive solution for parsing strings or vectors into vectors of a specified type. This functionality is crucial for handling data that may come in different formats but needs to be normalized into a consistent internal representation. The design of this utility is well-thought-out, offering flexibility through custom separators and parsing functions.
lucatrv
force-pushed
the
add_field_helper_functions
branch
from
df9b28f to
08ebb38
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (1)
- src/field_attributes.rs (29 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/field_attributes.rs
lucatrv
force-pushed
the
add_field_helper_functions
branch
from
08ebb38 to
e01ff98
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (1)
- src/field_attributes.rs (29 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/field_attributes.rs
| /// f64_or_string: Result<f64, String>, | ||
| /// } | ||
| /// | ||
| /// let s = r#" { "i64_or_f64": 1, "f64_or_string": 1 } "#; |
There was a problem hiding this comment.
I do not think 1 is a valid f64 number, as it has no valid representation in the floating point value specification. I think the parser should have failed. I understand this is not how serde works here, but this might be confusing. Can we do something about it? I don't think so. Can you think of any possible improvement here?
| /// let s = r#" { "i64_or_f64": "foo", "f64_or_string": "foo" } "#; | ||
| /// assert!(serde_json::from_str::<MyStruct>(s).is_err()); | ||
| /// ``` | ||
| pub fn deserialize_to_type_or_fallback<'de, D, T, F>( |
There was a problem hiding this comment.
I understand that this implementation was quite easy to implement this way, but I do not really like using the Result type for this matter. I wish we introduced another generic enum of our own to clearly designate the purpose (and the context), something like:
enum Either<FirstType, SecondType> {
First(FirstType),
Second(SecondType),
}What do you think about that?
| /// let a: MyStruct = serde_json::from_str(s).unwrap(); | ||
| /// assert_eq!(a.f64_or_string_lossy, Err(String::from("foo"))); | ||
| /// ``` | ||
| pub fn deserialize_to_type_or_string_lossy<'de, D, T>( |
There was a problem hiding this comment.
Should we have something like that but also returning just the byte array of the input instead? Perhaps, it could be more useful in some cases than a string? What do you think?
| let result = T::deserialize(value.clone()).map_err(|_| match value { | ||
| serde_value::Value::Bool(b) => b.to_string(), | ||
| serde_value::Value::U8(u) => u.to_string(), | ||
| serde_value::Value::U16(u) => u.to_string(), | ||
| serde_value::Value::U32(u) => u.to_string(), | ||
| serde_value::Value::U64(u) => u.to_string(), | ||
| serde_value::Value::I8(i) => i.to_string(), | ||
| serde_value::Value::I16(i) => i.to_string(), | ||
| serde_value::Value::I32(i) => i.to_string(), | ||
| serde_value::Value::I64(i) => i.to_string(), | ||
| serde_value::Value::F32(f) => f.to_string(), | ||
| serde_value::Value::F64(f) => f.to_string(), | ||
| serde_value::Value::Char(c) => c.to_string(), | ||
| serde_value::Value::String(s) => s, | ||
| serde_value::Value::Unit => String::new(), | ||
| serde_value::Value::Option(opt) => { | ||
| format!("{:?}", opt) | ||
| } | ||
| serde_value::Value::Newtype(nt) => { | ||
| format!("{:?}", nt) | ||
| } | ||
| serde_value::Value::Seq(seq) => format!("{:?}", seq), | ||
| serde_value::Value::Map(map) => format!("{:?}", map), | ||
| serde_value::Value::Bytes(v) => String::from_utf8_lossy(&v).into_owned(), | ||
| }); | ||
| Ok(result) |
There was a problem hiding this comment.
All the branches of this code lead to an immediate return, we can just
T::deserialize(value.clone()).map_err(|_| match value {
serde_value::Value::Bool(b) => b.to_string(),
serde_value::Value::U8(u) => u.to_string(),
serde_value::Value::U16(u) => u.to_string(),
serde_value::Value::U32(u) => u.to_string(),
serde_value::Value::U64(u) => u.to_string(),
serde_value::Value::I8(i) => i.to_string(),
serde_value::Value::I16(i) => i.to_string(),
serde_value::Value::I32(i) => i.to_string(),
serde_value::Value::I64(i) => i.to_string(),
serde_value::Value::F32(f) => f.to_string(),
serde_value::Value::F64(f) => f.to_string(),
serde_value::Value::Char(c) => c.to_string(),
serde_value::Value::String(s) => s,
serde_value::Value::Unit => String::new(),
serde_value::Value::Option(opt) => {
format!("{:?}", opt)
}
serde_value::Value::Newtype(nt) => {
format!("{:?}", nt)
}
serde_value::Value::Seq(seq) => format!("{:?}", seq),
serde_value::Value::Map(map) => format!("{:?}", map),
serde_value::Value::Bytes(v) => String::from_utf8_lossy(&v).into_owned(),
})| D: Deserializer<'de>, | ||
| T: Deserialize<'de>, |
There was a problem hiding this comment.
Why was this and other similar changes necessary? Did cargo fmt, or cargo clippy, or rustc complain about that?
| let result = T::deserialize(value.clone()).map_err(|_| match value { | ||
| serde_value::Value::Bool(b) => b.to_string(), | ||
| serde_value::Value::U8(u) => u.to_string(), | ||
| serde_value::Value::U16(u) => u.to_string(), | ||
| serde_value::Value::U32(u) => u.to_string(), | ||
| serde_value::Value::U64(u) => u.to_string(), | ||
| serde_value::Value::I8(i) => i.to_string(), | ||
| serde_value::Value::I16(i) => i.to_string(), | ||
| serde_value::Value::I32(i) => i.to_string(), | ||
| serde_value::Value::I64(i) => i.to_string(), | ||
| serde_value::Value::F32(f) => f.to_string(), | ||
| serde_value::Value::F64(f) => f.to_string(), | ||
| serde_value::Value::Char(c) => c.to_string(), | ||
| serde_value::Value::String(s) => s, | ||
| serde_value::Value::Unit => String::new(), | ||
| serde_value::Value::Option(opt) => { | ||
| format!("{:?}", opt) | ||
| } | ||
| serde_value::Value::Newtype(nt) => { | ||
| format!("{:?}", nt) | ||
| } | ||
| serde_value::Value::Seq(seq) => format!("{:?}", seq), | ||
| serde_value::Value::Map(map) => format!("{:?}", map), | ||
| serde_value::Value::Bytes(v) => String::from_utf8_lossy(&v).into_owned(), | ||
| }); |
There was a problem hiding this comment.
It looks like we can achieve the same result using either the deserialisation to serde_json::Value or implementing all these functions manually instead of using (and relying) on an old and, possibly, unmaintained crate such as serve-value. I also think that the less unreasonable dependencies there are in the dependency tree, the better it is for many various reasons. Not that I am strictly opposed to having it, I just don't (yet) see enough value in it.
Summary by CodeRabbit