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

Added a parent variable to a route so that it tells who its parent controller was (if any) #1097

Merged
merged 2 commits into from Mar 10, 2013

Conversation

Projects
None yet
3 participants
@dariocravero
Contributor

dariocravero commented Mar 10, 2013

Added a parent variable to a route so that it tells who its parent controller was (if any)
If it doesn't have a parent it will be nil.

See motivation here: https://github.com/dariocravero/padrino-autobind-parent-example

Thoughts?

/cc @postmodern

Darío Javier Cravero
Added a parent variable to a route so that it tells who its parent
controller was (if any).

If it doesn't have a parent it will be nil.

See motivation here:
https://github.com/dariocravero/padrino-autobind-parent-example
@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Mar 10, 2013

Member

Looks fine. Can we add a simple test checking the expected value of parent from inside a controller route?

Member

nesquena commented Mar 10, 2013

Looks fine. Can we add a simple test checking the expected value of parent from inside a controller route?

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Mar 10, 2013

Contributor

Yup, give me a sec

Contributor

dariocravero commented Mar 10, 2013

Yup, give me a sec

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Mar 10, 2013

Member

Awesome, looks good to me then. Thanks @dariocravero

Member

nesquena commented Mar 10, 2013

Awesome, looks good to me then. Thanks @dariocravero

nesquena added a commit that referenced this pull request Mar 10, 2013

Merge pull request #1097 from padrino/make-route-remember-parent
Added a parent variable to a route so that it tells who its parent controller was (if any)

@nesquena nesquena merged commit 005d9ed into master Mar 10, 2013

1 check was pending

default The Travis build is in progress
Details

@nesquena nesquena deleted the make-route-remember-parent branch Mar 10, 2013

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Mar 10, 2013

Contributor

Thanks for merging it, I realised I missed one case scenario though:

AutobindParentObject::App.controllers :bar, parent: :foo do
  get :index do
    "I'm index @ foo/bar and foo is #{@foo.id}."
  end

  get :index, parent: :quux do
    "I'm index @ foo/quux/bar, foo is #{@foo.id} and quux is #{@quux.id}."
  end
end

In that case, foo doesn't appear in index quux because the parent set on the route takes precedence.

Will apply a patch to mix them accordingly!..

Contributor

dariocravero commented Mar 10, 2013

Thanks for merging it, I realised I missed one case scenario though:

AutobindParentObject::App.controllers :bar, parent: :foo do
  get :index do
    "I'm index @ foo/bar and foo is #{@foo.id}."
  end

  get :index, parent: :quux do
    "I'm index @ foo/quux/bar, foo is #{@foo.id} and quux is #{@quux.id}."
  end
end

In that case, foo doesn't appear in index quux because the parent set on the route takes precedence.

Will apply a patch to mix them accordingly!..

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Mar 10, 2013

Member

Ah OK good catch, thanks @dariocravero

Member

nesquena commented Mar 10, 2013

Ah OK good catch, thanks @dariocravero

@postmodern

This comment has been minimized.

Show comment
Hide comment
@postmodern

postmodern Mar 10, 2013

Contributor

This will make my life so much better! Thanks @dariocravero.

Contributor

postmodern commented Mar 10, 2013

This will make my life so much better! Thanks @dariocravero.

@postmodern

This comment has been minimized.

Show comment
Hide comment
@postmodern

postmodern Mar 10, 2013

Contributor

@dariocravero question, if @foo is nil will padrino do a halt 404 or is that still my responsibility?

Contributor

postmodern commented Mar 10, 2013

@dariocravero question, if @foo is nil will padrino do a halt 404 or is that still my responsibility?

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Mar 11, 2013

Contributor

@postmodern well, that depends on what Foo.find does I guess, does it throw an exception if it doesn't find the record or something? I reckon it's up to you then how you want to handle it but if you think it's worth it we can put the halt 404 in place. Feel free to fork and add to the example, I'll move that as a padrino-recipe at some point - it kind of makes sense to be there :)

Contributor

dariocravero commented Mar 11, 2013

@postmodern well, that depends on what Foo.find does I guess, does it throw an exception if it doesn't find the record or something? I reckon it's up to you then how you want to handle it but if you think it's worth it we can put the halt 404 in place. Feel free to fork and add to the example, I'll move that as a padrino-recipe at some point - it kind of makes sense to be there :)

@postmodern

This comment has been minimized.

Show comment
Hide comment
@postmodern

postmodern Mar 11, 2013

Contributor

For DataMapper Foo.get returns nil and find is Enumerable#find. I known ActiveRecord's find raises an exception. I have no clue how MongoMapper behaves.

Contributor

postmodern commented Mar 11, 2013

For DataMapper Foo.get returns nil and find is Enumerable#find. I known ActiveRecord's find raises an exception. I have no clue how MongoMapper behaves.

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Mar 11, 2013

Contributor

OK, then I guess we could put those conditionals in there and then provide two code examples, one that throws a 404 and the other that doesn't?

Contributor

dariocravero commented Mar 11, 2013

OK, then I guess we could put those conditionals in there and then provide two code examples, one that throws a 404 and the other that doesn't?

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