Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Updated Tutorial #203

Merged
merged 16 commits into from Oct 5, 2012

Conversation

Projects
None yet
4 participants
Contributor

MiguelMadero commented Oct 1, 2012

There was a lot of yak shaving involved. I'm new with node, jake and the like so I had to do a lot of unrelated digging and fixed (or will log) a few bugs that I found along the way.

Please have a look and let me know what do you think, specially of the changes made to the templates, cli and other areas.

I also ended up with a long list of 'todos' that I'm not sure if I should log as issues or where would be the best place to discuss them. I'm pasting them here to start the discussion instead of just flooding the issue list.

Bugs:

  • "host/controller/" doesn't work as "host/controller" when using routes
  • Metrics isn't working (fails when starting geddy)
  • Geddy (server) fails without warning if there's another server running on 4000. I think it should crash or at least show a warning and move to a different port (only in dev)
  • If we have spaces when calling geddy jake fails because in cli.js we're calling cmds.split(' '). This is common when we have more than one property when calling scaffold, resource or model.
  • If the generator fails midway, there's no indication of th error (I have an inprogress fix calling jake differently to listen the error event and WARN the user and ask them to use the -d flag for more details)
  • It looks like if we change a rule in the model it affects the way we retrieve old date. For exmaple, I saved a TODO with a short title, then add a rule {min: 5} and the title of old TODOs is empty. There's no easy way to retrieve and compare using Geddy so that someone can fix the old, now invalid, TODOs.
  • Why does the mongodb-wrapper has to be there as a LOCAL module. Could it be globally installed?
  • If no default property is given Geddy will use id as the display property. I think we should pick the first property instead of id which is rarely useful. Ids should be internal and using it as a the default display property is misleading, I think Geddy should strive to use good defaults or force them to be explicit.

To re-test again with the HEAD

  • Review this issue and update the documentation once it's fixed.
    mde#79
    • The ajax PUT returns 302 to 'todos' and that returns 405
      possibly others

I'm happy to follow this up. I just wanted to start wrapping the tutorial first and to start getting some input in some of the other things I found.

Contributor

kieran commented Oct 1, 2012

Was thinking about the trailing slash in routes thing. My instinct is to strip trailing slashes before they hit the router since I can't think of a case where a trailing slash would be meaningful. Thoughts?

On Monday, 1 October, 2012 at 6:46 PM, Miguel Madero wrote:

There was a lot of yak shaving involved. I'm new with node, jake and the like so I had to do a lot of unrelated digging and fixed (or will log) a few bugs that I found along the way.
Please have a look and let me know what do you think, specially of the changes made to the templates, cli and other areas.
I also ended up with a long list of 'todos' that I'm not sure if I should log as issues or where would be the best place to discuss them. I'm pasting them here to start the discussion instead of just flooding the issue list.
Bugs:
"host/controller/" doesn't work as "host/controller" when using routes
Metrics isn't working (fails when starting geddy)
Geddy (server) fails without warning if there's another server running on 4000. I think it should crash or at least show a warning and move to a different port (only in dev)
If we have spaces when calling geddy jake fails because in cli.js we're calling cmds.split(' '). This is common when we have more than one property when calling scaffold, resource or model.
If the generator fails midway, there's no indication of th error (I have an inprogress fix calling jake differently to listen the error event and WARN the user and ask them to use the -d flag for more details)
It looks like if we change a rule in the model it affects the way we retrieve old date. For exmaple, I saved a TODO with a short title, then add a rule {min: 5} and the title of old TODOs is empty. There's no easy way to retrieve and compare using Geddy so that someone can fix the old, now invalid, TODOs.
Why does the mongodb-wrapper has to be there as a LOCAL module. Could it be globally installed?
If no default property is given Geddy will use id as the display property. I think we should pick the first property instead of id which is rarely useful. Ids should be internal and using it as a the default display property is misleading, I think Geddy should strive to use good defaults or force them to be explicit.

To re-test again with the HEAD
Review this issue and update the documentation once it's fixed. mde/geddy#79 (mde#79)
The ajax PUT returns 302 to 'todos' and that returns 405 possibly others

I'm happy to follow this up. I just wanted to start wrapping the tutorial first and to start getting some input in some of the other things I found.
You can merge this Pull Request by running:
git pull https://github.com/MiguelMadero/geddy tutorial
Or view, comment on, or merge it at:
mde#203
Commit Summary
Changed templates formatting
Changed templates formatting
Initial changes
Changed references to 'Mongo' for 'mongo' in docos because it doesn't…
Makes the memory adapter the default in config instead of adding it t…
Finished intro and scaffold tutorial
Merge branch 'master' of git://github.com/mde/geddy (http://github.com/mde/geddy)
Merge branch 'master' into tutorial
Updated the resource tutorial
Merge branch 'master' of git://github.com/mde/geddy (http://github.com/mde/geddy)
Merge branch 'master' into tutorial
Adds quick install
Changed the fileformat to unix
Merge branch 'master' into tutorial
Finished the resource tutorial
Made memory the defaultAdapter because the generator fails if we have…
Removed some TODOs
Did another pass and fixed some typos

File Changes
M Makefile (14)
M bin/cli.js (418)
M examples/related_models/app/models/box.js (2)
M examples/related_models/app/models/thing.js (2)
M examples/related_models/app/models/widget.js (2)
M examples/todo_app_coffee/app/models/todo.coffee (2)
M site/tutorial.html (534)
M templates/base/development.js (3)
M templates/base/production.js (10)
M templates/resource/model.ejs (2)
M templates/scaffold/controller.ejs (18)
M templates/scaffold/model.ejs (1)

Patch Links
https://github.com/mde/geddy/pull/203.patch
https://github.com/mde/geddy/pull/203.diff


Reply to this email directly or view it on GitHub (mde#203).

Contributor

mde commented Oct 2, 2012

That stupid change to DOS endings (my bad for actually fixing a bug inside Windows) has fucked up the ability to merge this, since it means the entire file has changed. I just pushed the change back to Unix, so you'll have an interesting pull when you sync from master. Lesson learned -- make sure Windows Vim converts back to Unix.

Also, do you want to file issues for all these bugs listed? Then we can discuss them individually.

Contributor

mde commented Oct 2, 2012

Oh, and I should say -- really kick-ass work on this!

Contributor

mde commented Oct 2, 2012

@kieran, I agree, I can't imagine a case where the trailing slash should actually be significant. Do you want to patch that? You do have commit access! :P If you don't get to it, I'll fix it. :)

Contributor

MiguelMadero commented Oct 3, 2012

@kieran and @mde I added a separate issue to discuss the trailing slash.
I'll add issues for the rest of the list as well.

@mde I fixed the DOS endings issue already so I had no problems with the merge.

@mde mde added a commit that referenced this pull request Oct 5, 2012

@mde mde Merge pull request #203 from MiguelMadero/tutorial
Updated Tutorial
f91fe7b

@mde mde merged commit f91fe7b into geddy:master Oct 5, 2012

Contributor

mde commented Oct 5, 2012

This looks good. Since our amazing new Web site update is almost ready, I'm going to wait until that lands to push this release and start talking it up. Now all the pressure is on you, @Techwraith!

Contributor

larzconwell commented Oct 5, 2012

👍 x 1000 Nice man!

Contributor

MiguelMadero commented Oct 5, 2012

@mde there're a few minor changes that would be nice to have before the next release. Could you have a look at mde#202

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment