Skip to content
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

Simplify SerdeError #1458

Merged
merged 3 commits into from
Feb 27, 2024
Merged

Simplify SerdeError #1458

merged 3 commits into from
Feb 27, 2024

Conversation

pitdicker
Copy link
Collaborator

While investigating all the CI failures on #1454 I noticed our serde code tries to convert LocalResults to a custom error type.
This can all be done in a much simpler way.

I did keep around a simple error enum because it allows reporting the invalid value.
It currently has only one variant for deserialization errors for UNIX timestamps. We could simplify it further, or leave room for adding more formats to deserialize.

Also included is the fix from #1396 for an invalid cast in the ts_seconds module for NaiveDateTime.

Copy link

codecov bot commented Feb 24, 2024

Codecov Report

Attention: Patch coverage is 65.85366% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 92.12%. Comparing base (02c68d6) to head (bc03a0f).
Report is 3 commits behind head on main.

Files Patch % Lines
src/lib.rs 0.00% 10 Missing ⚠️
src/datetime/serde.rs 90.47% 2 Missing ⚠️
src/naive/datetime/serde.rs 80.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1458      +/-   ##
==========================================
+ Coverage   92.11%   92.12%   +0.01%     
==========================================
  Files          40       40              
  Lines       18026    18000      -26     
==========================================
- Hits        16604    16583      -21     
+ Misses       1422     1417       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

}
}
}

/// Create a custom `de::Error` with `SerdeError::InvalidTimestamp`.
pub(crate) fn invalid_ts<E, T>(value: T) -> E
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three suggestions:

  • Maybe this function should be attached to SerdeError as a sort of (not quite) constructor, like SerdeError::invalid()?
  • (If not, this function should be defined above SerdeError)
  • Let's move this into the crate::serde inline module in lib.rs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Maybe this function should be attached to SerdeError as a sort of (not quite) constructor, like SerdeError::invalid()?

  • (If not, this function should be defined above SerdeError)

In my defence, that function looked very different when I started 😆. But I'll keep trying to put things in that order.

I like that with a standalone function only invalid_ts is public (within the crate), with the type just an implementation detail.

  • Let's move this into the crate::serde inline module in lib.rs?

Good suggestion, much more logical place. I added the move as an initial commit.

@pitdicker pitdicker merged commit a6c0c64 into chronotope:main Feb 27, 2024
34 of 35 checks passed
@pitdicker pitdicker deleted the serde_error branch February 27, 2024 14:37
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