Skip to content
This repository has been archived by the owner. It is now read-only.

Implement MessageSchema.event resolver #29

Merged
merged 4 commits into from Jul 3, 2017

Conversation

Projects
None yet
4 participants
@c-w
Copy link
Member

commented Jul 3, 2017

@c-w c-w requested review from Smarker and erikschlegel Jul 3, 2017

@Smarker

Smarker approved these changes Jul 3, 2017

Copy link
Contributor

left a comment

LGTM with small comments

* @param {string[]} params
* @returns {Promise.<object[]>}
*/
function executeQuery(query, params) { // eslint-disable-line no-unused-vars

This comment has been minimized.

Copy link
@Smarker

Smarker Jul 3, 2017

Contributor

I agree this needs 2 parameters rather than passing in an unnecessary object with query and params properties.

return new Promise((resolve, reject) => {
const eventId = args && args.messageId;
if (!eventId) {
reject('No event id to fetch specified');

This comment has been minimized.

Copy link
@Smarker

Smarker Jul 3, 2017

Contributor

If you would like I believe you can also just do return reject('No event id to fetch specified'); and it should still prevent the below lines from executing.

This comment has been minimized.

Copy link
@c-w

c-w Jul 3, 2017

Author Member

I'll do this as a follow-up.

}
});
})
.catch(err => reject(err));

This comment has been minimized.

Copy link
@Smarker

Smarker Jul 3, 2017

Contributor

I think you could do catch(reject); too right? The way you have it now is more explicit, so I like that way, too.

This comment has been minimized.

Copy link
@c-w

c-w Jul 3, 2017

Author Member

I'll do this as a follow-up.

@jcjimenez
Copy link
Contributor

left a comment

LGTM

@c-w c-w merged commit 094b1b4 into master Jul 3, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@erikschlegel
Copy link
Collaborator

left a comment

LGTM

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