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

Serde module macro #1396

Closed
wants to merge 5 commits into from
Closed

Conversation

pitdicker
Copy link
Collaborator

@pitdicker pitdicker commented Jan 31, 2024

Every (de)serializer we have for use with serde_with comes with a lot of boilerplate.
Each needs two modules (for a regular and optional variant), serialize and deserialize methods, a Visitor type and implementation, some documentation, and doc examples that serve as basic tests.

This is the perfect use case for a macro. The most important inputs for such a macro are:

  • the name of the two modules
  • the type that is to be serialized
  • the types that are to be deserialized
  • closures for the actual serialize function and deserialize visitors
  • docstrings for errors and examples

Because we can't concatenate idents in macro's it unfortunately needs a few more inputs to have everything covered.


A disadvantage of macro's is that it becomes a kind of DSL. I went with the most straightforward solution: to make it to look like a struct with all the macro arguments.

The advantage is that in my opinion it becomes much easier to maintain the code and spot errors in documentation or implementation. And that is the part were our bugs used to be, not in the boilerplate. I think the line count speaks for itself; the implementations are ca. a quarter of before.

There is no change in functionality, but thanks to the macro the documentation is more consistent.

I did not replicate all doc examples. In my opinion the doc example for the regular modules is important, because that is what you point serde_with at. But I doubt anyone clicks on the individual serialize and deserialize functions. I did extend the module documentation a bit to highlight how to use them though.

Actually found another bug in our deserialization from timestamps while working on this.

Copy link

codecov bot commented Jan 31, 2024

Codecov Report

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

Project coverage is 92.17%. Comparing base (0aa46dd) to head (c945153).

Files Patch % Lines
src/serde.rs 54.16% 33 Missing ⚠️
src/naive/datetime/serde.rs 86.11% 15 Missing ⚠️
src/datetime/serde.rs 92.85% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1396      +/-   ##
==========================================
+ Coverage   91.95%   92.17%   +0.22%     
==========================================
  Files          40       41       +1     
  Lines       18035    17013    -1022     
==========================================
- Hits        16584    15682     -902     
+ Misses       1451     1331     -120     

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

@djc
Copy link
Contributor

djc commented Apr 4, 2024

So I generally don't love macros like this that have fairly intricate DSL nature to them. I don't think it's all that natural to use a struct-like syntax for something more complex like this, so IMO this is not an improvement in its current shape at least.

@pitdicker
Copy link
Collaborator Author

The problem is that I also don't particularly like it, but if 80% of all the code I have to scroll through is boilerplate I have a hard time catching issues. By now I think we are in pretty good shape, but we only support a few limited 64-bit timestamps. Once we add a couple thousand lines more it is even harder to maintain.

Do you see another way to organize things or get rid of the boilerplate, besides using a macro with suboptimal syntax?

@djc
Copy link
Contributor

djc commented Apr 5, 2024

Do you see another way to organize things or get rid of the boilerplate, besides using a macro with suboptimal syntax?

Use an integration test to generate code, similar to the windows bindings stuff?

@pitdicker
Copy link
Collaborator Author

That could work. Not going to be easy; I'll look into it sometime.

@pitdicker pitdicker closed this Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants