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 multiple --prepare support #218

Merged
merged 6 commits into from Oct 13, 2019
Merged

Add multiple --prepare support #218

merged 6 commits into from Oct 13, 2019

Conversation

iamsauravsharma
Copy link
Contributor

Add support for multiple --prepare option. Corresponding --prepare option will be used for command for benchmarking according to position. If --prepare option is less than benchmark command then all other remaining benchmark command will use last prepare option ( This logic may not be best logic to implement out). If prepare option is more than command for benchmarking then extra --prepare option will not be used
Part of #216

Signed-off-by: Saurav Sharma <appdroiddeveloper@gmail.com>
@sharkdp
Copy link
Owner

sharkdp commented Oct 6, 2019

Thank you for your contribution.

If --prepare option is less than benchmark command then all other remaining benchmark command will use last prepare option ( This logic may not be best logic to implement out). If prepare option is more than command for benchmarking then extra --prepare option will not be used

I would solve both problems by issuing an error message if --prepare has not been specified exactly once or exactly N times. This way, we don't have to make these decisions. Either, we use one preparation-command for all benchmark-commands, or we have a 1:1 mapping of preparation-commands to benchmark-commands.

Signed-off-by: Saurav Sharma <appdroiddeveloper@gmail.com>
@iamsauravsharma
Copy link
Contributor Author

@sharkdp How would you like to raise error currently I am directly raising error by calling error function which is simple. And any suggestion over error message which was raised I think this is not best error message for this error

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 very much. I added some comments.

src/hyperfine/app.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/hyperfine/benchmark.rs Show resolved Hide resolved
src/hyperfine/benchmark.rs Outdated Show resolved Hide resolved
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!

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.

None yet

2 participants