-
-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add owned String branch to deserialize_option_number_from_string
#43
Conversation
WalkthroughThe project has embraced updates across its configuration and source code to support Rust version 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/field_attributes.rs (3 hunks)
Additional comments (4)
src/field_attributes.rs (4)
- 404-404: The addition of the
String(String)
variant to theNumericOrNull
enum is a significant enhancement. It allows for more flexible handling of string values during deserialization, particularly when converting between string representations and numeric types or identifying null values. This change aligns with the PR's objectives and improves the library's deserialization capabilities.- 414-417: The implementation of the
String(String)
variant in thedeserialize_option_number_from_string
function is correct and effectively handles the deserialization of owned string values. By matching against theString
variant and performing a non-empty check before attempting to parse, the function ensures that empty strings are correctly interpreted asNone
, and non-empty strings are parsed into the desired numeric type if possible.- 440-451: The usage of the
String(String)
variant within thewrap_option_number_from_string_fn
macro is consistent with its implementation in thedeserialize_option_number_from_string
function. This consistency is crucial for maintaining the expected behavior across different deserialization contexts that utilize this macro. The handling of empty and non-empty strings is appropriately managed, ensuring that the deserialization logic is robust and flexible.- 401-407: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
Consider organizing imports to improve readability. Grouping standard library imports together, followed by external crates, and then internal module imports can make the code easier to navigate and maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/field_attributes.rs (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/field_attributes.rs
…mily of functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
Cargo.toml
is excluded by!**/*.toml
Files selected for processing (4)
- .github/workflows/ci.yml (1 hunks)
- .github/workflows/nightly.yml (1 hunks)
- README.md (1 hunks)
- src/field_attributes.rs (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/field_attributes.rs
Additional comments not posted (3)
README.md (1)
20-20
: LGTM! It's good to see consistent documentation of Rust version support..github/workflows/ci.yml (1)
6-6
: LGTM! Ensuring CI configurations match the documented Rust version support is crucial for consistency..github/workflows/nightly.yml (1)
8-8
: LGTM! It's important to maintain consistency across all CI configurations with the project's Rust version support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (5)
- .github/workflows/ci.yml (1 hunks)
- .github/workflows/nightly.yml (1 hunks)
- Cargo.toml (1 hunks)
- README.md (1 hunks)
- src/field_attributes.rs (6 hunks)
Files skipped from review due to trivial changes (1)
- Cargo.toml
Files skipped from review as they are similar to previous changes (3)
- .github/workflows/ci.yml
- .github/workflows/nightly.yml
- src/field_attributes.rs
Additional comments not posted (1)
README.md (1)
20-20
: Consider adding a brief note explaining why the minimum Rust version was raised to1.61
, such as dependency requirements.
Thanks a lot! I sincerely apologise for the late response! |
This is somehow needed when I combine
serde-aux
withserde-dynamo
.Without this change I do have an error: "data did not match any variant of untagged enum NumericOrNull".
Summary by CodeRabbit
String(String)
to theNumericOrNull
enum for improved data processing.