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.

Sign up for GitHub

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

Jump to bottom

Add option to choose the units used for CLI & results output #80

Closed
jasonpeacock opened this issue Sep 9, 2018 · 7 comments
Closed

Add option to choose the units used for CLI & results output #80

jasonpeacock opened this issue Sep 9, 2018 · 7 comments
Labels
feature-request

Comments

@jasonpeacock
Copy link
Contributor

(a follow up to #71)

Currently the CLI automatically selects the units (seconds, milliseconds) based on the size of the mean value, but the results export is always in seconds.

Choosing the units will force for the CLI and results export units to always match, and allow users to specify the units they are most familiar with/integrate with their reporting systems.

The option could be -u --units <Seconds|Milliseconds>, and maybe extended to also include minutes (and hours)?

The units option value would be passed through to both format::format_duration_units for the CLI and ExportManager::write_results for the results export.
jasonpeacock added a commit to jasonpeacock/hyperfine that referenced this issue Sep 9, 2018
@jasonpeacock
Change markdown export to use seconds, closes sharkdp#71
08aeb10 
The markdown results exporter was using milliseconds, but the other
results exporters are using the default seconds unit.

* Change the markdown results exporter to also use the default seconds
unit of `BenchmarkResult`.

*This will change the markdown results for all users from milliseconds
to seconds, a UX breaking change.*

* Add unit tests for the markdown exporter to verify the output.

Issue sharkdp#80 proposes a new option to choose the units used for both the
CLI report and results export.
@jasonpeacock jasonpeacock mentioned this issue Sep 9, 2018
Closed
@sharkdp
Copy link
Owner

sharkdp commented Sep 9, 2018

Thank you for the feedback!

If this only affects the CLI and the Markdown export, this sounds good to me.

I'd suggest -u --time-unit <millisecond|second|...>.

@jasonpeacock
Copy link
Contributor Author

That makes sense. I'll start work on this when I finish #71.

jasonpeacock added a commit to jasonpeacock/hyperfine that referenced this issue Sep 15, 2018
@jasonpeacock
Change markdown export to use seconds, closes sharkdp#71
a231c79 
The markdown results exporter was using milliseconds, but the other
results exporters are using the default seconds unit.

Apply some refactoring so the same logic used to select units for the
CLI output can be used to select the units for the markdown exporter.

Now the units of the markdown exporter match that of the CLI.

When there are multiple benchmarks, the CLI will choose the
appropriate unit for each benchmark separately. In this case, the
markdown exporter will use the first benchmark to select the units for
all the results.

*This will change the markdown results for all users from milliseconds
to seconds, a UX breaking change.*

Add unit tests for the markdown exporter to verify the output.

Issue sharkdp#80 proposes a new option to choose the units used for both the
CLI report and results export.
jasonpeacock added a commit to jasonpeacock/hyperfine that referenced this issue Sep 15, 2018
@jasonpeacock
Change markdown export to use seconds, closes sharkdp#71
e912e0f 
The markdown results exporter was using milliseconds, but the other
results exporters are using the default seconds unit.

Apply some refactoring so the same logic used to select units for the
CLI output can be used to select the units for the markdown exporter.

Now the units of the markdown exporter match that of the CLI.

When there are multiple benchmarks, the CLI will choose the
appropriate unit for each benchmark separately. In this case, the
markdown exporter will use the first benchmark to select the units for
all the results.

*This will change the markdown results for all users from milliseconds
to seconds, a UX breaking change.*

Add unit tests for the markdown exporter to verify the output.

Issue sharkdp#80 proposes a new option to choose the units used for both the
CLI report and results export.
jasonpeacock added a commit to jasonpeacock/hyperfine that referenced this issue Sep 15, 2018
@jasonpeacock
Change markdown export to use seconds, closes sharkdp#71
596e689 
The markdown results exporter was using milliseconds, but the other
results exporters are using the default seconds unit.

Apply some refactoring so the same logic used to select units for the
CLI output can be used to select the units for the markdown exporter.

Now the units of the markdown exporter match that of the CLI.

When there are multiple benchmarks, the CLI will choose the
appropriate unit for each benchmark separately. In this case, the
markdown exporter will use the first benchmark to select the units for
all the results.

*This will change the markdown results for all users from milliseconds
to seconds, a UX breaking change.*

Add unit tests for the markdown exporter to verify the output.

Issue sharkdp#80 proposes a new option to choose the units used for both the
CLI report and results export.
@jasonpeacock jasonpeacock mentioned this issue Sep 15, 2018
Merged
@hashimaziz1
Copy link

hashimaziz1 commented Sep 28, 2018 •
edited

I would also look forward to this, but instead of --time-unit can shorter options be preferred like -t? Long options are a lot more work, and I don't understand why some CLI programs insist on preferring long options before all of the short alternatives have been exhausted.

@sharkdp
Copy link
Owner

sharkdp commented Sep 28, 2018

I'm okay with a short option. I just suggested -u (for "unit", there are no other units in hyperfine) instead of -t.

Long options are a hell of a lot of work, and I don't understand why some CLI programs insist on preferring long options before all of the short alternatives have been exhausted.

I typically add long command-line options to my programs because they have a self-documenting property which is great for usage in scripts.

Short options are great for interactive use of often-used features, but I tend to also be hesitant when adding new short flags because the namespace is really small and there might be better uses for a flag in the future. For rarely-used features (like hyperfines --export-* <FILE> flags), I think its perfectly fine to not have a short option because nobody will remember them anyway.

jasonpeacock added a commit to jasonpeacock/hyperfine that referenced this issue Oct 28, 2018
@jasonpeacock
Change markdown export to use seconds, closes sharkdp#71
0e9f6af 
The markdown results exporter was using milliseconds, but the other
results exporters are using the default seconds unit.

Apply some refactoring so the same logic used to select units for the
CLI output can be used to select the units for the markdown exporter.

Now the units of the markdown exporter match that of the CLI.

When there are multiple benchmarks, the CLI will choose the
appropriate unit for each benchmark separately. In this case, the
markdown exporter will use the first benchmark to select the units for
all the results.

*This will change the markdown results for all users from milliseconds
to seconds, a UX breaking change.*

Add unit tests for the markdown exporter to verify the output.

Issue sharkdp#80 proposes a new option to choose the units used for both the
CLI report and results export.
sharkdp pushed a commit that referenced this issue Oct 28, 2018
@jasonpeacock @sharkdp
Change markdown export to use seconds, closes #71
29804c5 
The markdown results exporter was using milliseconds, but the other
results exporters are using the default seconds unit.

Apply some refactoring so the same logic used to select units for the
CLI output can be used to select the units for the markdown exporter.

Now the units of the markdown exporter match that of the CLI.

When there are multiple benchmarks, the CLI will choose the
appropriate unit for each benchmark separately. In this case, the
markdown exporter will use the first benchmark to select the units for
all the results.

*This will change the markdown results for all users from milliseconds
to seconds, a UX breaking change.*

Add unit tests for the markdown exporter to verify the output.

Issue #80 proposes a new option to choose the units used for both the
CLI report and results export.
@jasonpeacock
Copy link
Contributor Author

working on this now, will use the suggested -u --time-unit <millisecond|second|...> options

jasonpeacock added a commit to jasonpeacock/hyperfine that referenced this issue Oct 30, 2018
@jasonpeacock
Add option to set time units for CLI & Markdown, closes sharkdp#80
193efbe 
Add option to set the time units to either `MilliSecond` or `Second`
for both CLI output and Markdown export, overriding the default logic.
jasonpeacock added a commit to jasonpeacock/hyperfine that referenced this issue Oct 30, 2018
@jasonpeacock
Add option to set time units for CLI & Markdown, closes sharkdp#80
53cc24e 
Add option to set the time units to either `MilliSecond` or `Second`
for both CLI output and Markdown export, overriding the default logic.
jasonpeacock added a commit to jasonpeacock/hyperfine that referenced this issue Oct 30, 2018
@jasonpeacock
Add option to set time units for CLI & Markdown, closes sharkdp#80
01bb700 
Add option to set the time units to either `MilliSecond` or `Second`
for both CLI output and Markdown export, overriding the default logic.
jasonpeacock added a commit to jasonpeacock/hyperfine that referenced this issue Oct 30, 2018
@jasonpeacock
Add option to set time units for CLI & Markdown, closes sharkdp#80
ec99c5e 
Add option to set the time units to either `MilliSecond` or `Second`
for both CLI output and Markdown export, overriding the default logic.
@jasonpeacock
Copy link
Contributor Author

PR submitted.

jasonpeacock added a commit to jasonpeacock/hyperfine that referenced this issue Nov 2, 2018
@jasonpeacock
Add option to set time units for CLI & Markdown, closes sharkdp#80
960499c 
Add option to set the time units to either `MilliSecond` or `Second`
for both CLI output and Markdown export, overriding the default logic.
@sharkdp sharkdp closed this as completed in f0b6f13 Nov 4, 2018
@sharkdp
Copy link
Owner

sharkdp commented Nov 21, 2018

Released in v1.4.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature-request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@sharkdp @jasonpeacock @hashimaziz1