initial import #1
Conversation
|
@ryanseys, could you take a look? |
|
Depends on googleapis/google-cloud-node#154 |
| return; | ||
| } | ||
| res.json(items.map(function(obj, i) { | ||
| obj.data.id = obj.key.path_.pop(); |
There was a problem hiding this comment.
We should add some pseudo getters for IDs and names to avoid this.
|
Could we rename this repo to appengine-nodejs-todos? gcloud-node sample apps should work without bits from GAE. |
| var gcloud = require('gcloud'), | ||
| datastore = gcloud.datastore; | ||
|
|
||
| var ds = new datastore.Dataset({ |
There was a problem hiding this comment.
You should be able to do the following once googleapis/google-cloud-node#154 is merged.
var ds = new datastore.Dataset();
@rakyll that's a good question, one way could be to make this sample to be portable, and put the App Engine specific parts behind flags. |
|
+1 for putting App Engine behind flags. |
|
Should rename CONTRIB.md to CONTRIBUTING.md to have the contributing guidelines pop up when a developer creates a PR. See: https://github.com/blog/1184-contributing-guidelines |
That should be raised against https://github.com/GoogleCloudPlatform/Template |
Okay, done. Should still be done here. |
|
Fixed contrib and lint issues, PTAL |
|
Could you add some comments to the code so that developers can at-a-glance see what the different parts of the code will be doing? Also, @rakyll suggested you use some helper methods for: obj.data.id = obj.key.path_.pop();and such... If you don't put them in helpers, could you at least comment on what they are meant to do? |
|
@ryanseys, let's discuss this issue on googleapis/google-cloud-node#171 We should provide a getter. |
|
PTAL bump gcloud deps, and added blueprint driven tests. |
| @@ -0,0 +1,42 @@ | |||
| var request = require('request'); | |||
| var hooks = require('hooks'); | |||
There was a problem hiding this comment.
hooks should be a dependency?
There was a problem hiding this comment.
I'm pretty sure this comes w/ dredd.
There was a problem hiding this comment.
Yes, it does https://github.com/apiaryio/dredd/blob/master/src/hooks.coffee
https://www.npmjs.org/package/hooks is another un-related library.
There was a problem hiding this comment.
And dredd add it on the fly when loading the hooks dynamically:
https://github.com/apiaryio/dredd/blob/master/src/add-hooks.coffee#L27
There was a problem hiding this comment.
Ah, ok. I was strictly looking at dependencies. Weird way to do it but I can't complain. 👍
There was a problem hiding this comment.
proxyquire FTW,
That's what they recommend in their doc, so I'm not guilty!
https://github.com/apiaryio/dredd/wiki/Writing-Hooks
There was a problem hiding this comment.
Add a comment explaining the magic. Otherwise, it may confuse the future maintainer.
|
FYI, tests are green on 0.10: |
|
LGTM Ship it. |
|
PTAL |
|
Looks good, go for it! |
|
Merged, thanks for the review. |
No description provided.