Remove From and Into test impls and create test-specific function for converting between Rust primitives and U256#3612
Conversation
Pull Request Test Coverage Report for Build 11826412804Details
💛 - Coveralls |
1f26364 to
7507836
Compare
apoelstra
left a comment
There was a problem hiding this comment.
ACK 750783604c05d7e4ce404e48ddb7a67886a51273; successfully ran local tests
tcharding
left a comment
There was a problem hiding this comment.
Thanks man, a few naming suggestions but looks good apart from that.
One small thing, can you make the commit brief log less than or equal to 54 charactars please. Long lines cause grief for other tools.
Remove From and Into test impls
blah blah create test-specific function blah blah
bitcoin/src/pow.rs
Outdated
There was a problem hiding this comment.
Perhaps call the module convert and the the functions u64_to_work, then at the call sites use convert::u64_to_work, and import with use crate::pow::convert.
Just an idea.
There was a problem hiding this comment.
That would look nicer. I do think that having something about testing in the module name would make it immediately clear that these are meant for testing only though, instead of having to look at the declaration and seeing the test flag. Any reason we shouldn't limit it to just testing-related code?
There was a problem hiding this comment.
Definitely only test code, and fair point about having 'test' in the name. Lets stick with your original module name (even though we try not to use 'util'). Perhaps drop the convert_.
7507836 to
48b32bc
Compare
The removed impls have been replaced with test-specific functions for converting between Rust primitives and U256
48b32bc to
33b9d69
Compare
Created a new
test-utilsmodule for these functions since it needs to be visible for tests in theblockmodule that were also using the previousFromandIntoimpls.Fixes #3160