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

Show elapsed time in minutes if >= 60 secs #5456

Merged
merged 2 commits into from
May 2, 2018
Merged

Show elapsed time in minutes if >= 60 secs #5456

merged 2 commits into from
May 2, 2018

Conversation

LukasKalbertodt
Copy link
Member

In large projects with long compile times, seeing "428.65 secs" isn't as clear to humans as seeing the number of minutes (and seconds).

Old:

Finished dev [unoptimized + debuginfo] target(s) in 2.23 secs
Finished dev [unoptimized + debuginfo] target(s) in 63.94 secs
Finished dev [unoptimized + debuginfo] target(s) in 428.65 secs

New:

Finished dev [unoptimized + debuginfo] target(s) in 2.23s
Finished dev [unoptimized + debuginfo] target(s) in 1m 3.94s
Finished dev [unoptimized + debuginfo] target(s) in 7m 8.65s

Note that I also changed secs to s, because 7 mins 8.65 secs and 7m 8.65 secs both look strange IMO. But if you disagree and you'd prefer secs, just tell me and I'll change it.

I didn't add a check for secs >= 3600 to print the time in hours. I hope this is not necessary...

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@LukasKalbertodt
Copy link
Member Author

That's what I get for using the GitHub web editor to hack on Cargo...

Anyway, now I fixed the problem and ran cargo test locally without failures. Now it should hopefully be fine.

@matklad
Copy link
Member

matklad commented May 2, 2018

@bors r+

Thanks!

I think that this might break some tools that try to parse this time, but this is a human-readable output, and we don't guarantee its stability.

@bors
Copy link
Contributor

bors commented May 2, 2018

📌 Commit f7773d8 has been approved by matklad

@bors
Copy link
Contributor

bors commented May 2, 2018

⌛ Testing commit f7773d88eb2befee1cdd0cb67c0fce2c726e0847 with merge b21b44a09ea657e42bd6749d6c4a009747d15d5e...

@bors
Copy link
Contributor

bors commented May 2, 2018

💔 Test failed - status-travis

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented May 2, 2018

☔ The latest upstream changes (presumably #5432) made this pull request unmergeable. Please resolve the merge conflicts.

In large projects with long compile times, seeing "400.65 secs" isn't as
clear as seeing the number of minutes (and seconds).
@LukasKalbertodt
Copy link
Member Author

Rebased

@alexcrichton
Copy link
Member

@bors: r=matklad

@bors
Copy link
Contributor

bors commented May 2, 2018

📌 Commit 3572813 has been approved by matklad

@bors
Copy link
Contributor

bors commented May 2, 2018

⌛ Testing commit 3572813 with merge 420f9ec...

bors added a commit that referenced this pull request May 2, 2018
Show elapsed time in minutes if >= 60 secs

In large projects with long compile times, seeing "428.65 secs" isn't as clear to humans as seeing the number of minutes (and seconds).

**Old**:
```
Finished dev [unoptimized + debuginfo] target(s) in 2.23 secs
Finished dev [unoptimized + debuginfo] target(s) in 63.94 secs
Finished dev [unoptimized + debuginfo] target(s) in 428.65 secs
```

**New**:
```
Finished dev [unoptimized + debuginfo] target(s) in 2.23s
Finished dev [unoptimized + debuginfo] target(s) in 1m 3.94s
Finished dev [unoptimized + debuginfo] target(s) in 7m 8.65s
```

Note that I also changed `secs` to `s`, because `7 mins 8.65 secs` and `7m 8.65 secs` both look strange IMO. But if you disagree and you'd prefer `secs`, just tell me and I'll change it.

I *didn't* add a check for `secs >= 3600` to print the time in hours. I *hope* this is not necessary...
@bors
Copy link
Contributor

bors commented May 2, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: matklad
Pushing 420f9ec to master...

@bors bors merged commit 3572813 into rust-lang:master May 2, 2018
@pravic
Copy link

pravic commented May 9, 2018

The seconds' fraction is redundant with minutes, imho, but it's too late.

@matklad
Copy link
Member

matklad commented May 9, 2018

but it's too late.

It's never to late for a PR :)

@daschl
Copy link
Contributor

daschl commented May 9, 2018

@pravic agreed, without the fractions it would look cleaner!

@ordovicia
Copy link
Contributor

I agree, too.
I am going to send a PR if anybody else has not been working on this yet.

bors added a commit that referenced this pull request May 10, 2018
Does not print seconds fraction with minutes

As discussed in #5456 (comment),
seconds fraction seems unnecessary when the elapsed time is reported in minutes.
@ehuss ehuss added this to the 1.27.0 milestone Feb 6, 2022
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.

9 participants