Skip to content
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

Work in progress fix for #538 #588

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brocktimus
Copy link
Contributor

More details in #538. This is just extracted from a monkey patch I've used briefly in an application as discussed there.

This obviously needs testing but I'm unsure best / most appropriate way to go about it. It requires a lot more "stuff" than currently exists in the test suite (I think...?).

I also thought it should go into its own name space or something for other optimizations? I'm guessing something similar could be written for other AR relations and potentially Mongoid as well.

Any thoughts / advice appreciated!

@steveklabnik
Copy link
Member

Could you rebase this, please? I've updated the travis file to actually build the correct rubies, and I'd like to see if it passes. Thanks!

@brocktimus
Copy link
Contributor Author

Done. I'm still not sure on best way to test this. Any ideas?

@stouset
Copy link

stouset commented Sep 22, 2014

Is this dead?

Having Draper blow away preloaded associations is a massive hit to performance.

@brocktimus
Copy link
Contributor Author

I'm still around, but haven't heard anything back since rebasing. Test fail on Rails 3 but I think we just skip that since it's super unsupported now. Still pretty unsure about testing however. i'll write a test for an idea I have now.

EDIT: Just looked and remembered the reason it was hard to test is at the moment the suite doesn't seem to have any models defined for associations and stuff. I'll have a bit of a look into the dummy app and see what I can do though, but any help would be greatly appreciated.

@jcasimir
Copy link
Member

Seems really interesting but we need test support / documentation.

@brocktimus
Copy link
Contributor Author

Based on #538 is this one also invalid if #decorate is being removed?

Otherwise I can look at it more just might need a bit of advice in best way of testing it. Most of the current testing stuff didn't seem to be dependent on having a full blown AR setup which this seems to start to require.

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

Successfully merging this pull request may close these issues.

None yet

5 participants