Refactor: Create uninstall submodule #6557
Conversation
|
r? @dwijnand (rust_highfive has picked a reviewer for you, use r? to override) |
There was a problem hiding this comment.
I like this change because cargo_install is a big module (871 lines, second best to cargo_compile, at 884) and this moves just shy of 400 lines of it into a 250 line shared module and a smaller 150 line uninstall module.
I'd love if we could find a better name for the shared module, and I have a small question on a publically defined struct field.
For these repo refactoring PRs I'd love if a more senior maintainer could drop a note with their thoughts.
src/cargo/ops/mod.rs
Outdated
| mod fix; | ||
| mod lockfile; | ||
| mod registry; | ||
| mod resolve; | ||
| mod utils; |
There was a problem hiding this comment.
Can we find a better name? I generally try to avoid generic names like "utils" because they end up containing anything (because everything is intended to bring utility :D). But particularly in cargo seeing as we already have a cargo::util (and a cargo::sources::git::utils).
There was a problem hiding this comment.
Okay. I will have time to fix it.
There was a problem hiding this comment.
ops::utils -> ops::common_for_install_and_uninstall
Somewhat verbose name, but how about this?
src/cargo/ops/cargo_uninstall.rs
Outdated
| ) -> CargoResult<()> { | ||
| let mut to_remove = Vec::new(); | ||
| { | ||
| let mut installed = match metadata.v1.entry(pkgid) { |
There was a problem hiding this comment.
@dwijnand I needed to refer to v1 here, so I changed the field to public.
I am not familiar with Rust so much, so please tell me if there is a better way.
There was a problem hiding this comment.
Add a v1() "getter" method to metadata, that simply returns the v1 field.
There was a problem hiding this comment.
@dwijnand I'm sorry. Why is it not good to make the struct's field public?
I am sorry to hear it late. 😭
There was a problem hiding this comment.
I'm not sure precisely. I've seen advice against it, but I can't remember the motivation. I'd want to understand better myself too.
There was a problem hiding this comment.
I see. thank you for answering! 🎉
|
@dwijnand As I fixed it, would you see it again when there is time❓ |
|
I'm really enjoying your "common between install and uninstall" module name! :D. It's specific enough to avoid being a catch-all module, but it's long enough that I expect it'll find a nicer name sooner or later (hopefully not utils!). So it's a good enough working title to land this, for me. Thank you for the PR. @bors: r+ |
|
📌 Commit 6a197fb has been approved by |
Refactor: Create uninstall submodule Since 'uninstall' was found in the 'install' module, it split. And I moved duplicate functions to utils.
|
☀️ Test successful - checks-travis, status-appveyor |
Update cargo Pull in fix for #57774. 6 commits in ffe65875fd05018599ad07e7389e99050c7915be..907c0febe7045fa02dff2a35c5e36d3bd59ea50d 2019-01-17 23:57:50 +0000 to 2019-01-20 22:31:07 +0000 - Put mtime-on-use behind a feature flag. (rust-lang/cargo#6573) - Fix a typo in the unstable docs (rust-lang/cargo#6569) - Perhaps you meant: foo, bar or foobar (rust-lang/cargo#6550) - Refactor: Create uninstall submodule (rust-lang/cargo#6557) - Fix spurious Windows errors with switch_features_rerun. (rust-lang/cargo#6561) - Stop building on master on Travis. (rust-lang/cargo#6562) r? @Mark-Simulacrum
Since 'uninstall' was found in the 'install' module, it split.
And I moved duplicate functions to utils.