Common API modules #33
Conversation
dr-orlovsky
added
the
enhancement
There was a problem hiding this comment.
I think we should take some time to think about the self issue. Looking at the code, it seems that my assumption was correct and we should use &self instead.
Sorry about the comments outside of the review, I was confused by several notifications due to you mentioning me in each commit, so missed that there's PR for the whole thing.
| fn into_message(self) -> Message { | ||
| Message::from(consensus_serialize(&self.into_inner())) | ||
| } | ||
| fn try_from_message(message: Message) -> Result<Self, Self::Error> { |
There was a problem hiding this comment.
Forgot to add in the other comment that &Message might be more useful as well.
There was a problem hiding this comment.
If we can use to_ instead of into_ for passing references; is there some convention for from_s passing references and not consuming the argument?
There was a problem hiding this comment.
Also, another important point: we use impl From to actually serialize many data structures into Message, and this is just a helper method. If we will pass a reference here, we will have to consume the argument still with the underlying From, so I don't understand the reason of changing self->&self in into/from_message functions here...
There was a problem hiding this comment.
I'm not aware of any convention for from. I've seen from_* function take values and references. Just to_*/into_* are differentiated. I suppose because Foo::from_something(&bar) immediately tells the reader what's going on, where bar.to_foo() wouldn't if not for the convention.
From could be implemented for references too. Implementing it for references only might be even better as it might help avoiding accidental clones.
There was a problem hiding this comment.
Tried to move from selfs to &self, however got plenty of problems throughout the code; in particular many parts depend on further into_/from_ consuming selfs, so I will keep it this way for now and at some point later, when I will have more time, revisit this and refactor the most of codebase
|
|
||
| #[derive(Debug, Display)] | ||
| #[display_from(Debug)] | ||
| pub enum Error { |
There was a problem hiding this comment.
I expect the API to be in flux for some time, so making the enum private and bundling it into a newtype struct might improve forward compatibility for the time being.
There was a problem hiding this comment.
I think just adding #[non_exhaustive] (which I simply forgot here) will do the job?
There was a problem hiding this comment.
An advantage of struct over non_exhaustive is that it can change its internal representation without breaking backwards compatibility. This may be important in the early stage. Once possible errors are well-understood, making the enum public (possibly with non_exhaustive) will be fine.
WIP on ZMQ APIs: fixed marker traits Refactoring API after @Kixunil suggestion: Step 1 Refactoring API after @Kixunil suggestion: Step 2 Refactoring API after @Kixunil suggestion: Step 3 Refactoring API after @Kixunil suggestion: Step 4 Refactoring API after @Kixunil suggestion: Step 5 Refactoring API after @Kixunil suggestion: Step 6 Refactoring API after @Kixunil suggestion: Step 7 Refactoring API after @Kixunil suggestion: removing markers for bp and rgb Refactoring API after @Kixunil suggestion: Step 8
dr-orlovsky
force-pushed
the
feat-api
branch
2 times, most recently
from
64031fe
to
c800df4
Compare
| pub enum Native { } | ||
|
|
||
| /// Strategy used for custom implementation of data structure encoding | ||
| pub trait Other { type Strategy: Clone + fmt::Debug + fmt::Display; } |
There was a problem hiding this comment.
The Clone + fmt::Debug + fmt::Display bounds aren't necessary because Strategy is only type-level marker and in the case of empty enums, it can't be even instantiated.
| #[cfg(feature="use-rgb")] | ||
| impl<T> MessageEncode for strategy::Holder<T, strategy::RGBStrategy> | ||
| where T: csv::serialize::Network { | ||
| type Error = Error; |
There was a problem hiding this comment.
The general convention is to not have fixed Error type everywhere, but use the original error type. type Error; would be unnecessary otherwise, since the trait could require conversion to some specific type. std::io::{Read, Write} uses fixed errors (std::io::Error) and from my experience, it's quite unfortunate in these cases:
- Application needs to do something smart around errors (real life example: translate messages)
no_stdis out of question- Inconvenient when transferring more information is needed (e.g. both OS code and file name)
So my recommendation is to prefer the original error and let the client of the library unify it if needed.
No description provided.