Performance degradation in content_columns: respond_to instead of rescue #775

Merged
merged 3 commits into from Jan 14, 2012

Conversation

Projects
None yet
2 participants
Contributor

dnagir commented Jan 8, 2012

rescue causes a slowdown for ORMs that don't have content_columns

See https://gist.github.com/2bea805664bfb6491d1d#gistcomment-73931

rescue-ing from everything is pretty weird thing to.

(Should probably consider not using it at all here).

Owner

justinfrench commented Jan 8, 2012

Looks good, agree rescuing from everything is crappy, especially now, where no one particularly knows what this was routing around. The only tiny change I'd love to see is that we've used klass instead of clazz elsewhere in the code. If your're willing to go down the rabbit hole with the raise stuff, go for it, otherwise I'll pull in what's here.

Contributor

dnagir commented Jan 8, 2012

Ok. I'll get rid of rescue altogether and will add a commit here.

On 08/01/2012, at 16:06, Justin Frenchreply@reply.github.com wrote:

Looks good, agree rescuing from everything is crappy, especially now, where no one particularly knows what this was routing around. The only tiny change I'd love to see is that we've used klass instead of clazz elsewhere in the code. If your're willing to go down the rabbit hole with the raise stuff, go for it, otherwise I'll pull in what's here.


Reply to this email directly or view it on GitHub:
#775 (comment)

Contributor

dnagir commented Jan 8, 2012

I got rid of the rescue. This broke only 2 specs that were referring to a model that doesn't exist.
I suppose it was a mistake in the spec.

Have a look the commit.

Cheers.

Contributor

dnagir commented Jan 8, 2012

If the specs are correct then we'll have to handle the case where string.constantize throws NameError if no constant exists (probably by checking somehow if it indeed exists).

Owner

justinfrench commented Jan 8, 2012

@dnagir model_name can originate from an object (where constantize will work fine, in the case of semantic_form_for @post), but also from the name of an object (where constantize will fail, in the case of semantic_form_for :post), so I think that we maybe need to rescue from NameError, which is probably what the original catch-all rescue was supposed to do.

We could also check if the constant exists beforehand, but I think that fails in dev mode with auto loading?

Contributor

dnagir commented Jan 8, 2012

semantic_form_for :post will work as expected as long as Post constant is defined. It won't work with semantic_form_for :something_undefined.

The question is: should it? If yes, then I'll rescue from the NameError and revert back the spec.

Owner

justinfrench commented Jan 8, 2012

@dnagir agreed, that's the question. Rails certainly allows you to use :post when Post is not defined. Formtastic is a little bit more fussy about having real objects, but at this stage, I'm pretty sure it's still allowed. There's nowhere else we're making that check right now, so I think it has to be done somewhere. Thoughts?

Contributor

dnagir commented Jan 9, 2012

I think for now I'll revert back the specs and handle NameError more explicitly. Maybe somebody in the future will come up with something better.

Owner

justinfrench commented Jan 14, 2012

@dnagir is this ready to merge in?

Contributor

dnagir commented Jan 14, 2012

Yeah, I believe so.

@justinfrench justinfrench added a commit that referenced this pull request Jan 14, 2012

@justinfrench justinfrench Merge pull request #775 from dnagir/rescue-content_columns
Performance degradation in content_columns: respond_to instead of rescue
cae97bf

@justinfrench justinfrench merged commit cae97bf into justinfrench:master Jan 14, 2012

Owner

justinfrench commented Jan 14, 2012

pulled in, thanks!

Contributor

dnagir commented Jan 21, 2012

A note about catching the exception.

Rails 3.2 has a method to handle that: 'safe_constantize'

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