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 support for cargo bench (Fixes #9) #12

Merged
merged 2 commits into from
Mar 6, 2019
Merged

Add support for cargo bench (Fixes #9) #12

merged 2 commits into from
Mar 6, 2019

Conversation

cbourjau
Copy link
Owner

This should fix the issue at hand but I might have killed some corner
cases. I deleted some error messages which don't look as if they were
actually sound anyways. What do you think, @barskern ?

This should fix the issue at hand but I might have killed some corner
cases. I deleted some error messages which don't look as if they were
actually sound anyways.
Copy link
Collaborator

@barskern barskern left a comment

Choose a reason for hiding this comment

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

This is a nice refactor and feature!

I think the new error message is a bit generic, what do you think?

[] => Err(err_msg("No suitable build artifacts found.")),
[the_one] => Ok(the_one),
the_many => Err(format_err!(
"Found several artifact candidates: \n :{:?}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should still print an error messages which suggests ways to solve the error (when many artifacts are found). E.g "Please use --test [...]".

Copy link
Owner Author

Choose a reason for hiding this comment

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

I am out of time for today, but will look into this in the next few days

@barskern
Copy link
Collaborator

We should probably also add some info about this in the README

@cbourjau
Copy link
Owner Author

Thanks already, I'm out of time for today, but will take a closer look at those --test flags in the coming days.

@velvia
Copy link

velvia commented Feb 23, 2019

@cbourjau thanks for doing this! This will be very helpful to many of us hoping to integrate cargo bench with other profiling tools.

@barskern
Copy link
Collaborator

barskern commented Mar 6, 2019

@cbourjau if you want we could merge this now and I could fix up the error message in a separate PR? I fully understand that you have other things which take time. As I currently have some available time, I wouldn't mind doing it. 😊

@cbourjau
Copy link
Owner Author

cbourjau commented Mar 6, 2019 via email

@barskern barskern merged commit 7f3c229 into master Mar 6, 2019
@barskern barskern deleted the add-cargo-bench branch March 10, 2019 00:36
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.

3 participants