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

More human readable elapsed time #857

Closed
wants to merge 6 commits into from
Closed

More human readable elapsed time #857

wants to merge 6 commits into from

Conversation

xiaozhuai
Copy link

@xiaozhuai xiaozhuai commented Jan 12, 2023

std::cout << format_time_userfriendly(std::chrono::milliseconds(3600l * 1000l + 123l)) << "\n";
std::cout << format_time_userfriendly(std::chrono::milliseconds(3600l * 1000l + 5123l)) << "\n";
std::cout << format_time_userfriendly(std::chrono::milliseconds(24l * 3600l * 1000l + 60l * 8l * 1000l + 5123l)) << "\n";
std::cout << format_time_userfriendly(std::chrono::milliseconds(25l * 3600l * 1000l + 60l * 8l * 1000l + 5123l)) << "\n";
std::cout << format_time_userfriendly(std::chrono::milliseconds((1l + 24l * 7l) * 3600l * 1000l + 60l * 8l * 1000l + 5123l)) << "\n";

It will prints

1h 0.123s
1h 5.123s
1d 8m 5.123s
1d 1h 8m 5.123s
7d 1h 8m 5.123s

The delimiter currently is default to ' ', it can be '' and prints

1h0.123s
1h5.123s
1d8m5.123s
1d1h8m5.123s
7d1h8m5.123s

current log

Elapsed time to handle qt5-base:x64-osx: 8.504 min

8.504 min is confusing, that will be

Elapsed time to handle qt5-base:x64-osx: 8m 30.240s

@xiaozhuai xiaozhuai changed the title More humand readable elapsed time More human readable elapsed time Jan 12, 2023
@xiaozhuai
Copy link
Author

The CI fails, someone please help

@autoantwort
Copy link
Contributor

Yeah the ci is broken since #773

@BillyONeal
Copy link
Member

CI fixed in 17bb1b8, please merge

@xiaozhuai
Copy link
Author

CI fixed in 17bb1b8, please merge

I've merge to master and it fails with different problem

@BillyONeal
Copy link
Member

CI fixed in 17bb1b8, please merge

I've merge to master and it fails with different problem

That looks like a legitimate problem created by this PR, not a baseline issue. (You can download the diff and git apply it)

@xiaozhuai
Copy link
Author

That looks like a legitimate problem created by this PR, not a baseline issue. (You can download the diff and git apply it)

But OSX and Linux build passing. So odd.

@vicroms
Copy link
Member

vicroms commented Jan 13, 2023

The issue is the code formatting, you need to apply the diff generated by CI to format it correctly. Alternatively, if you have clang format installed you can run the format CMake target to format all the sources.

@xiaozhuai
Copy link
Author

The issue is the code formatting, you need to apply the diff generated by CI to format it correctly. Alternatively, if you have clang format installed you can run the format CMake target to format all the sources.

Got it, thx!

@dg0yt
Copy link
Contributor

dg0yt commented Jan 13, 2023

IMO the proposed output is still somewhat controversial with regard to readability. I wonder if there are any external examples of good practices.
There are also too many digits of little significance.
My wish list is probably like:

59.9 s
1:01 min
2:59 h
1d 2:49 h

@xiaozhuai
Copy link
Author

std::cout << format_time_userfriendly(std::chrono::milliseconds(3600l * 1000l + 123l)) << "\n";
std::cout << format_time_userfriendly(std::chrono::milliseconds(3600l * 1000l + 5123l)) << "\n";
std::cout << format_time_userfriendly(std::chrono::milliseconds(24l * 3600l * 1000l + 60l * 8l * 1000l + 5123l)) << "\n";
std::cout << format_time_userfriendly(std::chrono::milliseconds(25l * 3600l * 1000l + 60l * 8l * 1000l + 5123l)) << "\n";
std::cout << format_time_userfriendly(std::chrono::milliseconds((1l + 24l * 7l) * 3600l * 1000l + 60l * 8l * 1000l + 5123l)) << "\n";

For these cases, what format do you prefer?

@dg0yt
Copy link
Contributor

dg0yt commented Jan 13, 2023

I would cut the line for my examples at unit boundaries. For you initial examples, my personal proposal results in:

1:00 h
1:00 h
1:08 h
1d 1:08 h
7d 1:08 h

More "precision" is just noise.

But others should have a chance to give feedback, too.

@xiaozhuai
Copy link
Author

Let's hear what others have to say.

@xiaozhuai
Copy link
Author

xiaozhuai commented Jan 13, 2023

What about this

01:00:00.123
01:00:05.123
1d 00:08:05.123
1d 01:08:05.123
7d 01:08:05.123

@dg0yt
Copy link
Contributor

dg0yt commented Jan 13, 2023

What about this

No. I think this perfectly machine readable. But the PR title promises something else.

@autoantwort
Copy link
Contributor

autoantwort commented Jan 13, 2023

Personally I find 8.504 min not confusing and faster to read than 8m 30.240s 😅 But it could be shortened to 8.5 min

@xiaozhuai
Copy link
Author

Probably this problem is difficult to agree on, so I will close this pr.

@xiaozhuai xiaozhuai closed this Jan 13, 2023
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.

5 participants