modularizing server and introduce cli example. by stephenplusplus · Pull Request #6 · GoogleCloudPlatform/google-cloud-node-todos

stephenplusplus · 2014-09-26T19:22:01Z

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

stephenplusplus · 2014-09-26T19:22:32Z

I removed Gulp, as it seemed overkill for just this task.

but I wanted to add more! :)

Haha, no problem. I can add it back in :)

please, also the travis build should probably depends on it.

Can you add gulp back?

proppy · 2014-10-01T16:14:11Z

stephenplusplus · 2014-10-02T17:01:29Z

My recent updates resolved:

readme links to TodoMVC & Docker
modularize todo functionality into one file, shared by the CLI & express apps

changed api blueprint:

{
  "id": 42,
- "text": "do this",
- "done": false
+ "data": {
+   "text": "do this",
+   "done": false
+ }
}

Still to-do:

Bring back gulp

proppy · 2014-10-05T03:08:42Z

Thanks for making those changes! Will take a closer look at them next week ;)

proppy · 2014-10-08T18:07:44Z

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() { }, // ... }

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.

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.

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 👍

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

stephenplusplus · 2014-10-08T20:47:06Z

stephenplusplus@77fd19c made a few changes:

todo structure is flat.
dredd was upgraded to support an afterAll hook to delete all todos created during testing
tests don't use request to ping the API in the hooks, instead the todos module

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

proppy · 2014-10-15T19:00:14Z

maybe this should live under app/ WDYT?

Do you mean this individual file or the server directory? mv server apps?

stephenplusplus · 2014-10-15T20:01:45Z

Gulp is back, along with minor restructuring:

[mv] server/server.js -> server/index.js
[mv] server/test/test_hooks.js -> server/test/hooks.js
[rm] server/test/server_test.js

proppy · 2014-10-15T20:09:15Z

Is lowercase more the norm?

If yes, can you git mv instead so we have history?

It is the norm and sure.

stephenplusplus · 2014-10-15T20:24:28Z

Well, I squashed these down in hopes to make doing the git mv Gulpfile.js gulpfile.js traceable in history, but git doesn't seem to be so smart with the case conversion. It still ended up being a rm and add, apparently because I had to git mv G... g... -f.

Anyway, same changes, just one commit now.

proppy · 2014-10-15T20:28:47Z

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?

👍 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.

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

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.

stephenplusplus · 2014-10-15T21:14:03Z

Flattened!

proppy · 2014-10-15T22:28:23Z

README.md to match the parent?

proppy · 2014-10-15T22:37:40Z

LGTM, just some nits.

stephenplusplus · 2014-10-15T23:57:14Z

Updated!

proppy · 2014-10-16T01:08:15Z

I would put that at the end.

under 'Other Examples'

proppy · 2014-10-16T01:12:31Z

At some point I will need to stop commenting and hit that merge button :)

stephenplusplus · 2014-10-16T01:33:17Z

:) hey, they're all for the better!

stephenplusplus · 2014-10-16T01:34:20Z

Travis is bombing: https://travis-ci.org/GoogleCloudPlatform/gcloud-node-todos/builds/38111282

Is this anything I caused?

proppy · 2014-10-16T01:37:03Z

@stephenplusplus nop, don't worry about travis, because it relies on encrypted credentials it will only work on master

proppy · 2014-10-16T01:40:11Z

s/response/payload/? to avoid the confusion between res and response?

Sorry, I missed that the first time. Fixed!

proppy · 2014-10-16T01:59:04Z

Thanks!

modularizing server and introduce cli example.

stephenplusplus · 2014-10-16T02:01:47Z

Woo!

proppy · 2014-10-16T02:03:01Z

And so much congrats for not breaking the build :)
https://travis-ci.org/GoogleCloudPlatform/gcloud-node-todos/builds/38112579

stephenplusplus reviewed Sep 26, 2014
View reviewed changes

stephenplusplus force-pushed the master branch from a0a1349 to 176bf8a Compare September 29, 2014 21:36

proppy reviewed Oct 1, 2014
View reviewed changes

Comment thread .jshintrc

proppy Oct 1, 2014

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks :)

stephenplusplus force-pushed the master branch from 176bf8a to 024d766 Compare October 2, 2014 16:53

stephenplusplus force-pushed the master branch 2 times, most recently from 12fc020 to 992c328 Compare October 2, 2014 17:32

proppy reviewed Oct 8, 2014
View reviewed changes

stephenplusplus force-pushed the master branch from 45bc701 to 77fd19c Compare October 8, 2014 20:43

proppy reviewed Oct 15, 2014
View reviewed changes

stephenplusplus force-pushed the master branch from a11df56 to 9083aba Compare October 15, 2014 20:21

modularizing server and introduce cli example.

8763582

stephenplusplus force-pushed the master branch from 9083aba to 8763582 Compare October 15, 2014 20:22

proppy reviewed Oct 15, 2014
View reviewed changes

flatten structure

3832e55

fix cli example.

440441b

stephenplusplus referenced this pull request in proppy/gcloud-node-todos Oct 15, 2014

frontend: implement change

12f3fec

proppy reviewed Oct 15, 2014
View reviewed changes

Comment thread cli/readme.md Outdated

proppy Oct 15, 2014

Copy link
Copy Markdown

Contributor

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?

stephenplusplus added 3 commits October 15, 2014 19:54

remove gulp complexity

ad5b624

update dependencies.

e0e7fa7

no, seriously. README.

c306f4e

proppy reviewed Oct 16, 2014
View reviewed changes

stephenplusplus added 2 commits October 15, 2014 21:25

remove examples heading & link cli demo

7cc1270

fix up travis & lint + _handleResponse -> _handleApiResponse

5cf76b4

proppy reviewed Oct 16, 2014
View reviewed changes

response -> payload

aec9d08

proppy added a commit that referenced this pull request Oct 16, 2014

Merge pull request #6 from stephenplusplus/master

d659be0

modularizing server and introduce cli example.

proppy merged commit d659be0 into GoogleCloudPlatform:master Oct 16, 2014

Uh oh!

Conversation

stephenplusplus commented Sep 26, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenplusplus commented Oct 2, 2014

Uh oh!

proppy commented Oct 5, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenplusplus commented Oct 8, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenplusplus commented Oct 15, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenplusplus commented Oct 15, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenplusplus commented Oct 15, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

proppy commented Oct 15, 2014

Uh oh!

stephenplusplus commented Oct 15, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

proppy commented Oct 16, 2014

Uh oh!

stephenplusplus commented Oct 16, 2014

Uh oh!

stephenplusplus commented Oct 16, 2014

Uh oh!

proppy commented Oct 16, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!