Skip to content

Add an option to specify time format#102

Merged
jrfonseca merged 1 commit intojrfonseca:masterfrom
eltoder:feature/time-format
Nov 22, 2024
Merged

Add an option to specify time format#102
jrfonseca merged 1 commit intojrfonseca:masterfrom
eltoder:feature/time-format

Conversation

@eltoder
Copy link
Copy Markdown
Contributor

@eltoder eltoder commented Nov 19, 2024

str(float) prints 16 decimal places, which is too verbose and much more than the precision in the profiles. Print 7 digits by default and allow setting the format with --time-format.

@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 19, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.70%. Comparing base (4f75c58) to head (cc55157).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
gprof2dot.py 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #102      +/-   ##
==========================================
- Coverage   85.76%   85.70%   -0.06%     
==========================================
  Files           1        1              
  Lines        2452     2449       -3     
==========================================
- Hits         2103     2099       -4     
- Misses        349      350       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@eltoder eltoder force-pushed the feature/time-format branch from 24a8507 to 4750704 Compare November 19, 2024 00:12
Copy link
Copy Markdown
Owner

@jrfonseca jrfonseca left a comment

Choose a reason for hiding this comment

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

Otherwise looks good.

I actually wonder if --time-format option is worthwhile, but have no strong opinion either way.

Comment thread gprof2dot.py Outdated


MULTIPLICATION_SIGN = chr(0xd7)
TIME_FORMAT = "%.7g"
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Given TIME_FORMAT is not a constant, but rather a variable, please rename it to timeFormat.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

str(float) prints 16 decimal places, which is too verbose and much more
than the precision in the profiles. Print 7 digits by default and allow
setting the format with --time-format.
@eltoder eltoder force-pushed the feature/time-format branch from 4750704 to cc55157 Compare November 19, 2024 12:09
@eltoder
Copy link
Copy Markdown
Contributor Author

eltoder commented Nov 19, 2024

Otherwise looks good.

I actually wonder if --time-format option is worthwhile, but have no strong opinion either way.

I think picking a format that pleases everyone can be hard and it's easy to make it configurable, so we can avoid arguing about it.

@eltoder
Copy link
Copy Markdown
Contributor Author

eltoder commented Nov 21, 2024

@jrfonseca please take another look

btw, --time-format can also be used to address #58 -- if one wants to see units in the labels, they can add it to the format, e.g. --time-format=%.6gs.

@jrfonseca jrfonseca merged commit d5f801c into jrfonseca:master Nov 22, 2024
@jrfonseca
Copy link
Copy Markdown
Owner

btw, --time-format can also be used to address #58 -- if one wants to see units in the labels, they can add it to the format, e.g. --time-format=%.6gs.

Good point.

Merged. Thanks.

@eltoder eltoder deleted the feature/time-format branch November 22, 2024 14:07
@eltoder
Copy link
Copy Markdown
Contributor Author

eltoder commented Nov 22, 2024

Thank you

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