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 "Check external command version for compatibility" #319

Merged
merged 1 commit into from Oct 8, 2017

Conversation

Projects
None yet
2 participants
@bobbo
Copy link
Contributor

bobbo commented Oct 7, 2017

fixes #286

@bobbo bobbo force-pushed the bobbo:issue-286 branch 3 times, most recently from cdfda7e to 9116c19 Oct 7, 2017

@budziq
Copy link
Collaborator

budziq left a comment

The example is almost perfect @bobbo !
please find some mi or nits below

src/app.md Outdated
let stdout = String::from_utf8(output.stdout)?;
// `git --version` output: "git version x.y.z"
let version = stdout.split(" ").last().expect("Invalid command output");

This comment has been minimized.

@budziq

budziq Oct 7, 2017

Collaborator

lets avoid using expect here. you can add custom error message with ok_or_else(|| "Invalid command ...")?

src/app.md Outdated
let parsed_version = Version::parse(version)?;
if !version_test.matches(&parsed_version) {
bail!("Command version lower than minimum supported version")

This comment has been minimized.

@budziq

budziq Oct 7, 2017

Collaborator

how about adding the parsed version number to the bail! invocation?
bail! has format capabilities identical to the println!

@bobbo bobbo force-pushed the bobbo:issue-286 branch from 9116c19 to b4be175 Oct 8, 2017

@bobbo

This comment has been minimized.

Copy link
Contributor Author

bobbo commented Oct 8, 2017

Hi @budziq, I've made the changes you suggested

@budziq budziq merged commit a4f355d into rust-lang-nursery:master Oct 8, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@budziq

This comment has been minimized.

Copy link
Collaborator

budziq commented Oct 8, 2017

Well done! Thanks @bobbo !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.