Skip to content
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

Omit "ETA 0s" output in download tracker #3826

Closed
wants to merge 1 commit into from
Closed

Omit "ETA 0s" output in download tracker #3826

wants to merge 1 commit into from

Conversation

barafael
Copy link

When running rustup, the ETA is shown and continuously updated. After a download has finished, ETA is 0, and it is printed for each download (marked bold)

info: downloading component 'cargo'
8.0 MiB / 8.0 MiB (100 %) 5.0 MiB/s in 1s ETA: 0s
info: downloading component 'clippy'
info: downloading component 'rust-docs'
15.1 MiB / 15.1 MiB (100 %) 5.3 MiB/s in 2s ETA: 0s
info: downloading component 'rust-std'
24.3 MiB / 24.3 MiB (100 %) 5.3 MiB/s in 4s ETA: 0s

This change omits printing the ETA when it is 0s:

info: downloading component 'cargo'
8.0 MiB / 8.0 MiB (100 %) 5.5 MiB/s in 1s
info: downloading component 'clippy'
info: downloading component 'rust-docs'
15.1 MiB / 15.1 MiB (100 %) 6.0 MiB/s in 2s
info: downloading component 'rust-std'
24.3 MiB / 24.3 MiB (100 %) 5.6 MiB/s in 4s

Not a big change :) lmk what you think :)

Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, suggested a slightly different way of spelling it.

Comment on lines 195 to 197
(eta_h != Duration::ZERO)
.then(|| format!(" ETA: {}", eta_h.display()))
.unwrap_or_default()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer to formulate it like this:

match eta_h {
     Duration::ZERO => format_args!(""),
     _ => format_args!(" ETA: {}", eta_h.display()),
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, that doesn't work because the format_args! creates a temporary which doesn't survive the match scope

@djc
Copy link
Contributor

djc commented May 15, 2024

That didn't exactly work, maybe bind let eta_format = eta_h.display(); before the outer format!() call?

@barafael
Copy link
Author

barafael commented May 15, 2024

I think format_args only works as direct argument to format!. I can't get it to work in any other way at least. But yes, the original double format! is also not ideal

@djc
Copy link
Contributor

djc commented May 15, 2024

Fixed in #3827.

@djc djc closed this May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants