-
Notifications
You must be signed in to change notification settings - Fork 952
Make all utils functions and structs crate-private and cleanup unused functions #2787
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
c5bb3fb
to
88ff3d7
Compare
@kinnison @rbtcollins Could you please take a look? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the most part I think this is fine - one query, and a quick check from @rbtcollins before this cleanup can go in.
I particularly like that you found what looks like some dead code.
88ff3d7
to
474fb0c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks plausible to me. @rbtcollins are you okay for this to merge?
Conflict resolved, @rbtcollins Could you please take a look? |
Please don't merge into your branch, instead rebase onto master. If you need help with that, I can provide it. |
490fa57
to
988f21c
Compare
Rebased! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to introduce
#![deny(unreachable_pub)]
To all of our global lints - demo here, try turning it off and on
@rbtcollins I agree that we should introduce that lint, but should I open it after my current cleanup is done? Right now we have a few more modules of code that need to be modified. If we open it now, there will be a lot of code that needs to be changed that will be warned. Do you think it is acceptable for us to open it at the end? Because the cli module has a lot of code that needs to be changed and I will change it in my next PR. |
Added a to-do item to the issue. |
I had another question on CommandError :).. Re the lint: I think the lint is part of this bug; it doesn't have to be part of this PR - so it can certainly come in as another patch if it has a lot of dead code it picks up. Oh - but heres an idea: you could be building the modules you touch with it, even if you don't commit the lint, you commit the results of the cleanup of the lint to the modules so each module is only touched once. |
Is this it? #2787 (comment) I should have replied to you?
Good idea, just tried it in the utils mod and no warnings were reported. |
please squash the two commits together, then I think we merge this |
988f21c
to
b9e2eb6
Compare
squashed! Thanks again for your review! |
part of #2730
Because we use some methods in our tests, we need to set it to pub.