-
Notifications
You must be signed in to change notification settings - Fork 66
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
modularizing server and introduce cli example. #6
Conversation
}); | ||
}); | ||
|
||
gulp.task('test', ['dredd']); |
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.
I removed Gulp, as it seemed overkill for just this task.
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 wanted to add more! :)
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.
Haha, no problem. I can add it back in :)
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.
please, also the travis build should probably depends on it.
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.
Can you add gulp back?
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.
Done.
a0a1349
to
176bf8a
Compare
@@ -0,0 +1,18 @@ | |||
{ | |||
"bitwise": true, |
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.
thanks :)
176bf8a
to
024d766
Compare
My recent updates resolved:
Still to-do:
|
12fc020
to
992c328
Compare
Thanks for making those changes! Will take a closer look at them next week ;) |
})); | ||
}); | ||
} | ||
module.exports.getAll = getAll; |
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.
curious why you export them one by one instead of doing (beware, I'm not that up to date on latest Node.js trends):
module.exports {
getAll: function() {
},
// ...
}
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's certainly fine to do it that way, but common convention in my experience tends to keep indentation as close to the left wall as possible. Nesting inside of module.exports just adds a level that can be quickly removed by extracting the functions.
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.
what about having the exports as the top then, instead of below each function?
Or even module.exports.getAll = function() {}
I'm curious about which one is more idomatic those days.
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's one of those situations where you pick it and just be consistent in your own code. The only benefit to one over another is readability, but as these are all common ways of writing JavaScript, they should each offer the same readability to a newcomer.
If you prefer to look at:
module.exports.getAll = function() {
// ...
};
I'm happy to go that route 👍
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.
I always prefer to export everything at the bottom of the file. That way I can go to a file and immediately scroll to the end to see what it's exporting. I would suggest we do it like proppy first suggested:
module.exports = {
getAll: function() {
},
// ...
};
// EOF
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.
Will do.
45bc701
to
77fd19c
Compare
stephenplusplus@77fd19c made a few changes:
I caught a bug after upgrading gcloud to 0.8.0: googleapis/google-cloud-node#250, so after a merge and release of the fix, I'll update the gcloud dependency here to 0.8.1 |
@@ -0,0 +1,75 @@ | |||
'use strict'; |
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.
maybe this should live under app/
WDYT?
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.
Do you mean this individual file or the server directory? mv server apps
?
Gulp is back, along with minor restructuring:
|
@@ -0,0 +1,51 @@ | |||
'use strict'; |
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 lowercase more the norm?
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.
If yes, can you git mv instead so we have history?
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 the norm and sure.
a11df56
to
9083aba
Compare
9083aba
to
8763582
Compare
Well, I squashed these down in hopes to make doing the Anyway, same changes, just one commit now. |
|
||
# API | ||
See the [apps](//github.com/GoogleCloudPlatform/gcloud-node-todos/apps) directory for a full list of examples. |
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.
my only concern here is that the app directory doesn't include the server sample.
Maybe we could have a flat structure instead.
Have todos.js
, server.js
and cli/
at the root and tests/
what do you think?
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.
👍 although this might be uncovering some underlying confusion I'm having. I don't consider the server an app, rather the gcloud integration point, simply always running in the background for the FE implementations to ping.
It does make sense to move it out of server/
just to make it more visible from the frontpage.
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.
Yes, so maybe:
server.js
todos.js
package.json
tests/
cli/
And consider cli/
as a sub project with it's own README.md
and package.json
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.
Sounds good. If we get to a point that we have multiple (many) examples, we can always shift them into their own directory if we think that's necessary.
Flattened! |
@@ -0,0 +1,11 @@ | |||
# Command Line Example |
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.
README.md to match the parent?
LGTM, just some nits. |
Updated! |
|
||
[![Build Status](https://travis-ci.org/GoogleCloudPlatform/gcloud-node-todos.svg?branch=master)](https://travis-ci.org/GoogleCloudPlatform/gcloud-node-todos) | ||
|
||
TodoMVC backend using [gcloud-node](//github.com/GoogleCloudPlatform/gcloud-node). | ||
## Examples |
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.
I would put that at the end.
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.
under 'Other Examples'
At some point I will need to stop commenting and hit that merge button :) |
:) hey, they're all for the better! |
Travis is bombing: https://travis-ci.org/GoogleCloudPlatform/gcloud-node-todos/builds/38111282 Is this anything I caused? |
@stephenplusplus nop, don't worry about travis, because it relies on encrypted credentials it will only work on |
}); | ||
|
||
function _handleApiResponse(res, successStatus) { | ||
return function(err, response) { |
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.
s/response/payload/? to avoid the confusion between res
and response
?
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.
Sorry, I missed that the first time. Fixed!
Thanks! |
modularizing server and introduce cli example.
And so much congrats for not breaking the build :) |
Following up from googleapis/google-cloud-datastore#50, this sent the server files to a new
/server
directory, as well as introduced a CLI example a little more sophisticated than the one from the earlier PR.An easier to look at representation: https://github.com/stephenplusplus/gcloud-node-todos