Conversation
jcjimenez
commented
Jul 3, 2017
•
edited
edited
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.
LGTM with comments
return reject(`Could not find site with id ${siteId}`); | ||
} | ||
if (rows.length > 1) { | ||
return reject(`Got more than one (${rows.length}) site with id ${siteId}`); |
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.
This reject's statement might get confusing (suppose rows.length is 3 and siteId is 1):
Got more than one 3 site with id 1
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.
Good, point! But I think we should be safe since it will look more like this:
Got more than one (2) site with id d4d294ec-5b2c-4674-8f3f-e1eb7b8dbb47
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.
But I agree - let me work on making that more readable.
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.
Oh yea you are right! Didn't see the parentheses.
const siteCollection = Object.assign({}, {runTime: '' + (Date.now() - startTime), sites: [ site ]}); | ||
resolve(siteCollection); | ||
}) | ||
.catch(err => reject(err)) |
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.
You can also do catch(reject);
here.
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.
LGTM
// .catch(err => reject(err)) | ||
.then(rows => { | ||
if (rows.length < 1) { | ||
return reject(`Could not find site with id ${siteId}`); |
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.
Nit: could inline the return-reject like elsewhere if (...) return reject(...)
} | ||
|
||
const site = cassandraRowToSite(rows[0]); | ||
const siteCollection = Object.assign({}, {runTime: '' + (Date.now() - startTime), sites: [ site ]}); |
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.
This is implemented more generally in #34.