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

Update templates/scaffold/controller.ejs #219

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

Eraden commented Oct 10, 2012

If you are already using function, Model and self can be solved with the use of scopes.

@Eraden Eraden Update templates/scaffold/controller.ejs
If you are already using function, Model and self can be solved with the use of scopes.
db2e1e8
Contributor

Techwraith commented Oct 10, 2012

@mde what do you think of this one, I'm on the fence.

On Oct 10, 2012, at 12:31 AM, Adrian notifications@github.com wrote:

If you are already using function, Model and self can be solved with the use of scopes.

You can merge this Pull Request by running:

git pull https://github.com/Eraden/geddy patch-1
Or view, comment on, or merge it at:

mde#219

Commit Summary

Update templates/scaffold/controller.ejs
File Changes

M templates/scaffold/controller.ejs (26)
Patch Links

https://github.com/mde/geddy/pull/219.patch
https://github.com/mde/geddy/pull/219.diff

Reply to this email directly or view it on GitHub.

Contributor

larzconwell commented Oct 10, 2012

This has a few errors currently, the variable is called <%= names.property.singular %>, but in multiple places you're calling <%= names.constructor.singular %>. The difference is the contructors name is CamelCase while the properties are cameCase.

Eraden commented Oct 10, 2012

You are right, i didn't notice that. But I don't have any error in my project.

I suppose that if you charge on line 3 property to constructor problem will be fixed

Contributor

larzconwell commented Oct 10, 2012

I get errors if I do this, just change line 3 to the constructor style, then we'll let @mde decide if we should merge it.

Contributor

mde commented Oct 10, 2012

Although using a constructor-level 'self' reference reference works in this case, I don't like it as a general approach because it fails if you reuse this object as a prototype for another constructor, or as a mixin. (So this would actually break if someone tried to use this same pattern in their Application controller.)

As a general rule, I much prefer to keep the 'self' reference local to the called method, so you can be absolutely sure that 'self' is the current 'this' that the method is being invoked on.

I think we'll give this one a miss -- but thanks for the pull-request, and I hope you'll keep them coming!

@mde mde closed this Oct 10, 2012

Eraden commented Oct 10, 2012

Maybe just replace self with something else like '<%=
names.constructor.plural %>' or '<%= names.property.plural %>'? This makes
code cleaner and easer to understand.

Z poważaniem
Woźniak Adrian

2012/10/10 Matthew Eernisse notifications@github.com

Although using a constructor-level 'self' reference reference works in
this case, I don't like it as a general approach because it fails if you
reuse this object as a prototype for another constructor, or as a mixin.
(So this would actually break if someone tried to use this same pattern in
their Application controller.)

As a general rule, I much prefer to keep the 'self' reference local to the
called method, so you can be absolutely sure that 'self' is the current
'this' that the method is being invoked on.

I think we'll give this one a miss -- but thanks for the pull-request, and
I hope you'll keep them coming!


Reply to this email directly or view it on GitHubhttps://github.com/mde/geddy/pull/219#issuecomment-9312483.

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