Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upExpose cargo failures #15
Conversation
White-Oak
added some commits
Feb 11, 2016
ebkalderon
referenced this pull request
Feb 12, 2016
Closed
Expose Cargo exit status if non-zero #13
ebkalderon
reviewed
Feb 12, 2016
| + | ||
| + if [ $? -ne 0 ]; then | ||
| + echo "--- Passed!" | ||
| + echo |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
ebkalderon
Feb 12, 2016
Member
There is something of a spacing issue here on line 83. Not very important, but my OCD just kicked in.
ebkalderon
Feb 12, 2016
Member
There is something of a spacing issue here on line 83. Not very important, but my OCD just kicked in.
ebkalderon
reviewed
Feb 12, 2016
| + Some("Cargo task failed!") | ||
| + } | ||
| + } | ||
| + Err(_) => Some("Failed to run Cargo!"), | ||
| } |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
ebkalderon
Feb 12, 2016
Member
I am considering using an if let statement in this case, since we are only using the contents of Ok but not Err. Tell me what you think of this:
let exec_result = command.stdout(Stdio::inherit())
.stderr(Stdio::inherit())
.output();
if let Ok(output) = exec_result {
if output.status.success() {
None
} else {
Some("Cargo exited with an error!")
}
} else {
Some("Could not launch Cargo!")
}
ebkalderon
Feb 12, 2016
Member
I am considering using an if let statement in this case, since we are only using the contents of Ok but not Err. Tell me what you think of this:
let exec_result = command.stdout(Stdio::inherit())
.stderr(Stdio::inherit())
.output();
if let Ok(output) = exec_result {
if output.status.success() {
None
} else {
Some("Cargo exited with an error!")
}
} else {
Some("Could not launch Cargo!")
}
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
ebkalderon
Feb 12, 2016
Member
@White-Oak Besides the two comments above, this looks pretty good. I'm going to build this on my local machine to try it out. Just fix the spacing on that one line, and it will be merge-ready.
|
@White-Oak Besides the two comments above, this looks pretty good. I'm going to build this on my local machine to try it out. Just fix the spacing on that one line, and it will be merge-ready. |
added a commit
that referenced
this pull request
Feb 13, 2016
ebkalderon
merged commit b0a3a86
into
amethyst:master
Feb 13, 2016
1 check passed
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
ebkalderon
Feb 13, 2016
Member
Thanks for the changes, @White-Oak! This and pull request #14 together bring us almost ready to closing issue #13. Merged.
|
Thanks for the changes, @White-Oak! This and pull request #14 together bring us almost ready to closing issue #13. Merged. |
White-Oak commentedFeb 11, 2016
cargo::callproperly returnsSome()(I still think it should beResultinstead ofOption) if cargo task has failed (exited with non-zero status).