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

Make SerializationError a newtype #865

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

Lorak-mmk
Copy link
Collaborator

Pre-review checklist

This PR consists of first 2 commits of #858
It was split out in order to merge it before #858, so that #862 and #851 can be rebased on top of it.

There are 2 changes made here:

  • changing dyn Any to dyn Error in SerializationError - it also supports downcasting, but is more useful interface
  • Making SerializationError a newtype instead of type alias. This prevents making conversion implementations too generic, like impl From<SerializationError> for QueryError, which after desugaring is impl From<Arc<dyn Error + Send + Sync>> for QueryError - and writing such implementations doesn't seem like a good idea.
  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

Error needs to be dynamic, so that users can use their own types, but
at the same time allow it to be stored in concrete type (like `QueryError`).
`dyn Error` supports downcasting, so it can be used instead of `Any`.
Implementations such as `impl From<SerializationError> for QueryError`
don't seem like a good idea when SerializationError is just an alias for
`Arc<dyn Error + Send + Sync>`. Make SerializationError a newtype to
avoid such general implementations.
@piodul piodul merged commit 2f78d46 into scylladb:main Dec 1, 2023
8 checks passed
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