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

add CLI option to identify a command with a custom name #330

Merged
merged 6 commits into from
Oct 15, 2020

Conversation

scampi
Copy link
Contributor

@scampi scampi commented Oct 9, 2020

CLI option --name can be used to define the name to identify a
command. If not passed, then the command itself is used. Commands
and names are paired in the same order: The first command executed gets
the first name passed as option.

Close #326

@sharkdp
Copy link
Owner

sharkdp commented Oct 13, 2020

Thank you very much for your contribution! I am planning to take a closer look soon.

@sharkdp
Copy link
Owner

sharkdp commented Oct 13, 2020

Before I take a closer look at the code, I have a few questions:

  • Does this affect the --export-* formats? (I think it should)
  • If so, would changes to the command name also be visible when plotting data via one of the scripts/…?

As for the PR, could you please update the man page, if possible?

Also, could you please add an entry to the "unreleased" section in CHANGELOG.md? The format for entries is:

- Description what has been changed, see #123 (@user)

where #123 links to the bug ticket and/or the PR and user is your username.

CLI option `--name` can be used to define the name to identify a
command. If not passed, then the command itself is used. Commands
and names are paired in the same order: The first command executed gets
the first name passed as option.

Close sharkdp#326
@scampi
Copy link
Contributor Author

scampi commented Oct 14, 2020

If so, would changes to the command name also be visible when plotting data via one of the scripts/…?

The output of the various exports have the custom names, and so does the chart created with a script, e.g., plot_histogram.py.

See new commits for the updated changelog and man page.

Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Thank you for the updates.

@@ -226,6 +226,16 @@ fn build_app() -> App<'static, 'static> {
when trying to benchmark output speed.",
),
)
.arg(
Arg::with_name("command-name")
.long("name")
Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer --command-name for the long option. If we have a short option anyway, the long option should have a self-documenting character IMO... e.g. for shell scripts. If we change it, the man page also has to be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good, see commit cc67788

Comment on lines +130 to +132
options.names = matches
.values_of("command-name")
.map(|values| values.map(String::from).collect::<Vec<String>>());
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should show an error message if there are more names than commands:

hyperfine 'echo a' -n a -n b

Having too few is okay, but probably not really intended in most cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I now return an error in that case, see 9ed5f0d

Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Thank you for the updates!

@sharkdp sharkdp merged commit 25f8129 into sharkdp:master Oct 15, 2020
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.

Feature request: Named commands
2 participants