Remove wildcard imports#2892
Conversation
Pull Request Test Coverage Report for Build 9600493574Details
💛 - Coveralls |
99f4c79 to
d66510d
Compare
Pull Request Test Coverage Report for Build 9600797335Details
💛 - Coveralls |
tcharding
left a comment
There was a problem hiding this comment.
The prelude changes look good so far
There was a problem hiding this comment.
These ones are ok to leave in, they are enum variants.
There was a problem hiding this comment.
I was hoping this was the answer, there are a lot of these.
There was a problem hiding this comment.
Should we update CONTRIBUTING.md to reflect this enum variant policy in
Line 218 in 817e54f
There was a problem hiding this comment.
I think that's a good idea, add a policy that wildcard imports should not be used except in the three cases stated in #2875. I can add something to that effect and send it for review.
|
Thanks, I'll keep working on this and mark the PR as ready for review when done. |
d66510d to
9c7e6cd
Compare
Pull Request Test Coverage Report for Build 9616691165Details
💛 - Coveralls |
9c7e6cd to
7a24f66
Compare
Pull Request Test Coverage Report for Build 9617084191Details
💛 - Coveralls |
|
I have removed all cases of wildcard imports that don't fit in one of the three exceptions given in the corresponding issue. As it happens they were all prelude imports. |
|
Ack 7a24f66 |
|
7a24f66 looks good to me. I'm unsure about the I wonder whether we should move/reexport everything from the prelude at the root so we can replace Anyway ACK this. |
|
After you rebase you need to run |
I did it for And then we could do the same in all crates. An obvious question is should be only import types used in the crate or just use a standard set for all crates? |
|
I'm happy to merge this as is and do the "remove prelude" module separately or as part of this, up to you @jamillambert |
|
I would prefer to merge this to address the wildcard import issue, and then remove prelude separately. It does seem like two separate issues. Sorry, I'll remember to run the just check-api and commit command next time, but let me know if you want me to add it to this PR before merging. |
No worries
It has to be added to get past CI, we can't merge unless all CI jobs are green. You'll need to rebase on maste also to fix conflicts. |
7a24f66 to
3a28ce5
Compare
Pull Request Test Coverage Report for Build 9659098277Details
💛 - Coveralls |
|
Rebased on master to fix conflict and CI fail. |
tcharding
left a comment
There was a problem hiding this comment.
ACK 3a28ce51562ba674e843717c2c3b7bf728c93873
|
Consider wrapping lines in commit messages at 72 characters bro (subject line should be 50 instead). |
apoelstra
left a comment
There was a problem hiding this comment.
ACK 3a28ce51562ba674e843717c2c3b7bf728c93873
|
Sorry, #2915 seems to have conflicted with this. Needs rebase. |
Wildcards have been replaced with what is actually used. In a couple of cases an additional use statement was added to the test module to import `DisplayHex` which is only used in test, but previously imported with the wildcard at the top.
3a28ce5 to
d099b9c
Compare
Pull Request Test Coverage Report for Build 9708833056Details
💛 - Coveralls |
|
Rebased to remove conflict and wrapped long lines in commit messages. |
These were accidentally removed in rust-bitcoin#2892 and not noticed because of gaps in all of our testing infrastructure. These gaps have been since fixed.
This patch replaces
prelude::*wildcard imports with the types actually used. In a couple of casesDisplayHexwas previously imported by the wildcard but was only used in the test module, an additional import was added to the test module instead of at the top where it causes an unused import warning.Close: #2875