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

Added default implementations for 'Serializer' and 'Deserializer' trait by raising error if not overridden #2849

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AlexSherbinin
Copy link

No description provided.

@oli-obk
Copy link
Member

oli-obk commented Oct 30, 2024

Can you talk a bit about your motivation for this change? Ideally with some links to examples that are hard, annoying or footgunny without this?

@AlexSherbinin
Copy link
Author

  1. Default method implementations already exist for the u128 and i128 types.
  2. There is a significant boilerplate involved with using the Impossible serializer in SerializeTuple, SerializeSeq, and similar cases.

I created this PR while transitioning this crypto library from an internal trait for hash function parameter serialization to Serde, and encountered the aforementioned issue with boilerplate.

@oli-obk
Copy link
Member

oli-obk commented Oct 30, 2024

  1. Default method implementations already exist for the u128 and i128 types.

That's only due to them being introduced later, so we can't add them without a breaking change or a default impl

2. There is a significant boilerplate involved with using the Impossible serializer in SerializeTuple, SerializeSeq, and similar cases.

These are rather special cases. Most serializers will want to implement all of them, and with default impls it's annoying to make sure you got them all

@AlexSherbinin
Copy link
Author

These are rather special cases. Most serializers will want to implement all of them, and with default impls it's annoying to make sure you got them all

Rust analyzer has code action to implement all trait members, even with default impl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants