Conversation
373ad72 to
ce17bc7
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
UnixShell::cargo_home_str, Nu::cargo_home_str, and Xonsh::cargo_home_str return only owned strings through cargo_home_str_with_home.
ce17bc7 to
a672820
Compare
|
@djc I would like to provide some extra context regarding this part of the changes: it has been a follow-up of my request #5091 (comment) in a previous attempt of decoupling the rcfile-related logic from This is needed because the new uninstallation logic will attempt to remove the rcfile entries from both the |
| #[cfg(unix)] | ||
| { | ||
| let home_dir = process.home_dir(); | ||
| for sh in shell::get_available_shells(process) { | ||
| let source_cmd = sh.source_string(cargo_home, home_dir.as_deref())?; | ||
| // Check more files for cleanup than normally are updated. | ||
| do_remove_from_path( | ||
| &source_cmd, | ||
| &sh.rc_candidates(home_dir.as_deref(), process), | ||
| )?; | ||
| } | ||
| unix::remove_legacy_paths( | ||
| cargo_home, | ||
| home_dir.as_deref(), | ||
| &shell::legacy_paths(process).collect::<Vec<_>>(), | ||
| )?; | ||
| } | ||
| #[cfg(windows)] | ||
| { | ||
| let environment = process | ||
| .registry_environment_key() | ||
| .context("Failed opening Environment key")?; | ||
| do_remove_from_path(cargo_home, &environment)?; | ||
| } |
There was a problem hiding this comment.
Nit: You can use cfg_select!{} for this.
| format!( | ||
| pre_uninstall_msg!(), | ||
| cargo_home = display_cargo_home(process)? | ||
| cargo_home = display_cargo_home(&cargo_home, process.home_dir().as_deref()) |
There was a problem hiding this comment.
Question: Judging from the immediate result, it seems like this call begs to be a new type declaration like the following...
struct CargoHomeDisplay { cargo_home: ..., home_dir: ... }
impl Display for CargoHomeDisplay { ... }However I am wondering how you are planning to fit the current refactoring change into the final implementation of #5056. Ideally the APIs should stay as close as possible to the final shape. Would you mind elaborating a bit on that point (as well as whether you think the above suggestion makes sense WRT your final goal)? 🙏
4 participants
#5091 was not a complete decouple of
Process's state from the related install/uninstall helpers.So, this PR does:
Cowusage ofcargo_home_str_with_home,cargo_home_str, as the caller immediately constructsStringProcess' states, e.g.cargo_homeand other, allowing for re-using the resolution result.remove_legacy_source_command, as the same logic is already done indo_remove_from_pathIn the meantime, this the first of a series of refactors around uninstall/install/self-replacing logic.