Conversation
don't know how to test the output yet.....
The formatted result would be like this. Thanks @rami3l for helping me out on this small issue here. |
src/dist/download.rs
Outdated
| } | ||
|
|
||
| impl DownloadStatus { | ||
| fn style(&self, suffix: &str) -> ProgressStyle { |
There was a problem hiding this comment.
Nit: Consider extracting this to a standalone function and to be used above as well, since the only dependency on &self is self.name_width().
You can make this clearer by extracting this function in the first commit which is separate from other changes with a fix named_width = 13, and then add the name_width parameter.
src/dist/manifestation.rs
Outdated
| .replace(DEFAULT_DIST_SERVER, dl_cfg.tmp_cx.dist_server.as_str()); | ||
|
|
||
| let status = dl_cfg.status_for("rust"); | ||
| let status = dl_cfg.status_for("rust", 15); |
There was a problem hiding this comment.
I think given the fact that the only component name is "rust" in v1, we'd just do:
let component = "rust"; let status = dl_cfg.status_for(component, component.len());
src/dist/manifestation.rs
Outdated
| let max_name_width = update | ||
| .components_to_install | ||
| .iter() | ||
| .map(|component| new_manifest.short_name(component).len()) |
There was a problem hiding this comment.
Nit: This implementation implicitly relied on the fact that .short_name() will be used in ComponentBinary for the component name.
Suggest extracting a function ComponentBinary::(component: Component, manifest: &'a Manifest) and use that on both sides instead.
src/dist/download.rs
Outdated
| progress.set_style( | ||
| ProgressStyle::with_template( | ||
| "{msg:>13.bold} downloading [{bar:15}] {total_bytes:>11} ({bytes_per_sec}, ETA: {eta})", | ||
| &format!("{{msg:>{}.bold}} downloading [{{bar:15}}] {{total_bytes:>11}} ({{bytes_per_sec}}, ETA: {{eta}})", name_width) |
There was a problem hiding this comment.
Nit: I think it's clearer to use the new style format!() both here and in other places, e.g. "{{msg:>{name_width}.bold}} ...
If you change the hash in |
|
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. |
Cloud0310
force-pushed
the
install-message-fix
branch
from
455ce11 to
dfb927b
Compare
|
@rami3l Thanks for your code style suggestions, I refactored the code, and tested with @FranciscoTGouveia method. |
There was a problem hiding this comment.
The overall logic looks fine to me now. Nice!
It must be noted though, in terms of history manipulation in nontrivial rustup PRs like this, it will require some more work to be done.
Basically we use atomic commits here so if there's one refactoring plus one new feature in the total list of changes that'd be two commits (and preferably refactoring comes first).
I suggest you do the same thing with this PR: split it into one or two refactoring PRs beforehand, and finally introduce the new feature. After that we can go back to the implementation details.
Many thanks in advance 🙏
4 participants
Fix #4759 with dynamic name width.