util::amount: Add a FromStr-based serde deserializer#509
util::amount: Add a FromStr-based serde deserializer#509stevenroose wants to merge 1 commit intorust-bitcoin:masterfrom
Conversation
|
Doesn't compile with 1.29.0 etc |
dr-orlovsky
left a comment
There was a problem hiding this comment.
utACK, but with fixes for 1.29 compilation errors
db0c602 to
dc54199
Compare
|
Fixed those MSRV errors and rebased. |
sgeisler
left a comment
There was a problem hiding this comment.
utACK dc54199c46169329500a27e43f8ac769ebfd172f
I think we should revisit the FromStr impl at some point and make it work properly (e.g. without spaces, like c-lightning would require it) at least if we still want to include and not deprecate it (I'm not sure if really that useful tbh, see my comment in #593).
src/util/amount.rs
Outdated
There was a problem hiding this comment.
Adding this bound is a breaking change, no?
There was a problem hiding this comment.
This trait is a "private" one and should not be used outside of the crate. However, this is not enforced (yet), so if somebody is using it against the recommendation, this may break their code.
dr-orlovsky
left a comment
There was a problem hiding this comment.
Needs rebase and replacement of all std:: to core::
d00b252 to
61ce753
Compare
|
Fixed and rebased. |
|
EDIT: Is it so it works with Also I was playing with a solution to #729. I pushed a draft PR (#730) that does the same as this PR but uses the same pattern as |
|
Needs rebase. Hi @stevenroose , we are gearing up for a major release soon. Otherwise, this might have to wait for 0.29 |
|
Hey mate, this PR is currently marked as part of the 0.29 milestone, would you like to get this functionality in before the 0.29 release? |
|
No response from @stevenroose so I'm going to remove the milestone from this one. |
|
This looks stale and I'd rather add Also is this actually needed in |
|
Closing as stale. Reopen when ready. |
Related to #501.