Replies: 5 comments
-
Thank you for such a detailed review. The main reason is to have these derives in bitcoin projects where I do not want to rely on any external additional crates from outside of bitcoin community for this trivial functionality. I will comment on the specifics tomorrow after thorough reading each of the points. |
Beta Was this translation helpful? Give feedback.
-
|
Beta Was this translation helpful? Give feedback.
-
The base of derive and error/from was written by @elichai because rust bitcoin community would never integrate those two crates you are mentioning (ask Andrew Poelstra and other maintainers why, you can spend months negotiating and writing comments, literally :). So this code was a part of @elichai's derive_wrapper crate; however I needed more, so made this version which I heavily use in LNP/BP Core Lib. It will take days of my work to change it on any other dependencies, and its not critical at all, so I will not spend my time on it before the first release. Unfortunately I had to move to syn 1.+, so my MSRV version is higher than in rust-bitcoin currently, but in a matter of a year or so it they will match and I hope rust-bitcoin will start using this crate. |
Beta Was this translation helpful? Give feedback.
-
No problem, I guess I don't know something they know. I don't want to push it more but since I intended to use |
Beta Was this translation helpful? Give feedback.
-
|
Beta Was this translation helpful? Give feedback.
-
I fail to see a strong reason to use
Display
andError
derives in this crate. There'sdisplaydoc
forDisplay
andthiserror
forError
. The examples aren't very convincing either:Example 1 of
Display
suggests using a different trait for implementing, which is a bad idea especially in case ofDebug
- it practically breaks the contract of theDisplay
trait. (ImplementingDebug
usingDisplay
would make more sense logically.)Example 2 suggests allocation which is again inefficient and it doesn't make sense to write a function returning owned
String
to implementDisplay
when one could implementDisplay
instead and use.to_string()
to get the string.The remaining
Display
examples are pretty much whatdisplaydoc
supports.thiserror
supports all the features ofError
derive in this crate plus also backtraces (I personally don't use backtraces but it's nice to have it available.)Further looking at some other things:
s!()
as very useful. If your program needs to allocate strings so often that you want to save 7 characters from the name.to_owned()
it is likely that you may want to redesign the program to allocate less in the first place. Methods are generally considered easier to work with than macros/"normal function" by many people, including me. (Prime examples:try!()
was turned into?
operator andawait
was implemented as postfix operator as well.) Finally.to_owned
is more readable to the newbies to a project as they don't have to look up whats!()
means.map!
and co are useful, though there's alreadyhmap!
for instance. (It looks dead but is there really anything relevant to change? It's simple and works as advertised.)InetAddr
with Tor support is great, the question is why it's not intorut
itself? It would make more sense to me. Also I'm not sure if its a good idea to name itInetAddr
could be confusing since such name exists instd
(not hugely against it)Service
toFutures
?Wrapper
trait? In my experience newtypes are used in two ways: to protect an invariant and to distinguish meanings of underlying values. In the former caseWrapper
trait must not be used as it does no checking (TryFrom<Inner>
,Into<Inner>
are idiomatic), in the latter case, I found that writingMyNewtype::new(42)
is much more readable than42.into()
I avoid implementingFrom<Inner> for Newtype
because of this. For conversion fromNewtype
toInner
simpleinto_inner
method orFrom<Newtype> for Inner
are more standard in the ecosystem.AsAny
doesn't have blanket impl? I also find it quite surprising that you need to useAny
. It's very rarely used. :)Holder
is being published it might be worth checking if it can be used with references well and think if there's a better name for it. I'm not even sure if it's useful to publish it because orphan rules might wreck its usefulness - a macro might be better.Beta Was this translation helpful? Give feedback.
All reactions