Simplifying macros #356
Simplifying macros #356
Conversation
Codecov Report
@@ Coverage Diff @@
## master #356 +/- ##
==========================================
- Coverage 85.74% 85.62% -0.12%
==========================================
Files 40 40
Lines 7483 7597 +114
==========================================
+ Hits 6416 6505 +89
- Misses 1067 1092 +25
Continue to review full report at Codecov.
|
|
Ok so sometimes converting into and from the inner object is useful. F.e. when interfacing with code that doesn't know any bitcoin conventions. I also did it before with FFI's. Apart from that, I'd ACK. Aaand it would be good to have in 0.22 perhaps since it's quite a big breaking change. But not a big deal if it's lateron. |
|
Would really love to see it merged. Can I help with adding the discussed changes to the PR? |
|
I kinda left it for a while to get feedback on if people even want that (especially because of the complications in the follow up PR with the derive macros) |
|
I think that the level of current macro abuse in rust-bitcoin is too high and any more idiomatic code should be welcomed |
|
I feel a bit double with all this. I think it'd be good to have 2 PRs: one that simply removes all impls that can be derived. Because that PR is a no-brainer. But removing methods and replacing them with Rust traits is more tricky. I remember as a new user that I often lacked "simple" or "basic" methods. And using traits, while nifty and cool, is less intuitive. A |
39ee0e0 to
c9be13b
Compare
You're right. I removed all controversial parts and left the impl ones. |
|
FYI CI failures are because of #398 |
|
Needs rebase. |
|
I honestly don't know how, but somehow the changes I reverted before(#356 (comment)) came back after the last rebase, I might've had an old version locally, although I'm not quite sure how. Anyhow I modified it to look back as the original, and you can check yourself, the hash after the first rebase is: c9be13b0935fa3f4b2f570119000b77015dafaa3 and the current hash is: 77589d2e122f63768fe7fde880078e439875842d so: |
|
I think I'm looking at the latest code and there are still missing |
|
Added the missing impls. |
ef5e244 to
02ddb27
Compare
|
rebased |
|
utACK 02ddb27 |
fails (first commit does not compile) |
02ddb27 to
fdd6f4f
Compare
This is the first step in an attempt to simplify the macros we use in this crate.
I tried removing anything that can be derived. and removing the general functions implementation that might made sense on crates like
rust-secp256k1for ffi reasons but less so here.cc #352
I'm trying to "Modernize" some of the great code @apoelstra wrote way before any of us ever heard of rust :)
The next stage is when these are simple enough I can write a custom derive crate for the rest of the things.
Sadly a thing that can greatly simplify the Index operations was is only stable from 1.28 rust-lang/rust#35729
The breaking changes are:
as_ptetc. implementations fromChainCodeandFingerprint(as I said before I don't think they make any sense here).#[repr(C)]from the Uints.anything else here shouldn't be considered a breaking change (exact implementations of traits like
Hashetc aren't supposed to be stable anyway)