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

Make `code` on `ExecuteGoalResult` optional #586

Merged
merged 2 commits into from Nov 29, 2018

Conversation

Projects
None yet
3 participants
@cdupuis
Copy link
Contributor

cdupuis commented Nov 29, 2018

Omitting code means successas does code: 0.

@cdupuis cdupuis requested a review from ddgenome Nov 29, 2018

@cdupuis cdupuis changed the title Autofix: tslint Make `code` on `ExecuteGoalResult` optional Nov 29, 2018

@ddgenome
Copy link
Member

ddgenome left a comment

Should clarify in TypeDoc about how to return success and failure.

if (!result.code) {
return true;
}
return result.code === 0;

This comment has been minimized.

@ddgenome

ddgenome Nov 29, 2018

Member

You could just return false here.

@@ -70,10 +70,32 @@ export interface ExecuteGoalResult extends GoalDetails {
* 0 is success; non-zero exit codes will mark the goal as failed,

This comment has been minimized.

@ddgenome

ddgenome Nov 29, 2018

Member

Should probably add that 0 or undefined is success.

This comment has been minimized.

@ddgenome

ddgenome Nov 29, 2018

Member

And that you should throw an error on failure.

This comment has been minimized.

@ddgenome

ddgenome Nov 29, 2018

Member

For the ExecuteGoalResult TypeDoc, something like:

/**
 * Result from a successful goal execution allowing you to provide an optional success message.
 * If the goal is unsuccessful, throw a description `Error`.
 */

@cdupuis cdupuis force-pushed the optional-code branch from f94a602 to 42cd989 Nov 29, 2018

@ddgenome
Copy link
Member

ddgenome left a comment

LG

@atomist-bot

This comment has been minimized.

Copy link
Contributor

atomist-bot commented Nov 29, 2018

Pull request auto merged by Atomist.

  • 1 approved review by @ddgenome
  • 2 successful checks

[atomist:generated] [auto-merge:on-approve]

@atomist-bot atomist-bot merged commit e92ee2c into master Nov 29, 2018

2 checks passed

license/cla Contributor License Agreement is signed.
Details
sdm/atomist/atomist-sdm Atomist Software Delivery Machine goals: all succeeded
Details

@atomist-bot atomist-bot deleted the optional-code branch Nov 29, 2018

atomist-bot added a commit that referenced this pull request Nov 29, 2018

Changelog: #586 to changed
[atomist:generated]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment