-
Notifications
You must be signed in to change notification settings - Fork 20
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 fetchGoalsFromPush method to read goals from subscription #559
Conversation
638e44f
to
fb07e5e
Compare
e94e7f0
to
26798f1
Compare
[atomist:generated] [atomist:autofix=typescript header]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. A couple questions.
@@ -35,6 +35,14 @@ import { | |||
import { goalKeyString } from "./sdmGoal"; | |||
import { goalCorrespondsToSdmGoal } from "./storeGoals"; | |||
|
|||
export function fetchGoalsFromPush(sdmGoal: SdmGoalEvent): SdmGoalEvent[] { | |||
const goals = sumSdmGoalEvents((sdmGoal.push as any).goals.filter(g => g.goalSetId === sdmGoal.goalSetId)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is sdmGoal.push
not typed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is typed to a different type. Internally we are using subscription types but the API is using public SdmGoalEvent
.
"description": "Build successful", | ||
"goalSetId": "c2e95b8a-61a5-4474-95de-7d918d0b5478", | ||
"preApproval": null, | ||
"preCondit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's some push.
"description": "Build successful", | ||
"goalSetId": "c2e95b8a-61a5-4474-95de-7d918d0b5478", | ||
"preApproval": null, | ||
"preCondit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"preCondit | |
assert(goals.every(g => g.push)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes no sense.
@@ -14,7 +14,7 @@ | |||
* limitations under the License. | |||
*/ | |||
|
|||
import assert = require("power-assert"); | |||
import * as assert from "power-assert"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aaawww yeah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your consideration of my humble comments.
Pull request auto merged by Atomist.
[atomist:generated] [auto-merge:on-approve] |
No description provided.