Update the coerced attribute in the aggrigate part for right caching. #6042

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@edtsech
Contributor
edtsech commented Apr 29, 2012

Patch for issue #2084 /cc @steveklabnik

@jeremyf
Contributor
jeremyf commented Apr 29, 2012

I'm very suspect about the part.instance_variable_set; Telling another object to update an instance variable indicates too much knowledge of the implementation of the part.

@edtsech can you refactor this?

@steveklabnik
Member

I don't have the power to merge code, and since you've already referenced #2084, I'll just keep that one closed.

@rafaelfranca
Member

I reopened that issue because this patch can be not accepted and we need to keep that issue tracked. When this one was accepted, I'll close the issue.

@steveklabnik
Member

I don't mind either way. I wasn't going to bother since they're all linked, but I'm new here, so I'll follow your lead. :)

@edtsech
Contributor
edtsech commented May 1, 2012

In last commit I've fixed two things.
First one is return old value from cache after object changes:

barney.address.street // => "Quiet Road"
barney.address_street = "Noisy Road"
barney.address.street // => "Quiet Road" but should be "Noisy Road"

and another one with type conversion (related to #2084):

barney.address = Address.new("May Street", "Chicago", "USA", "31")  
barney.address_house_number // => 31
barney.address.house_number // => "31" but should be 31

Now it works how it should work, in my opinion. Wdyt @josevalim ?

@josevalim
Member

This seems better but checking every time we read the aggregation if any changes were made seem slow. Ideally, we would override the mapping names to trigger this on change, but I am not exactly sure on how to do that.

@edtsech
Contributor
edtsech commented May 26, 2012

Thanks for your response @josevalim.
To keep things separately I've created pull request for the issue with caching #6498
And to solve the issue with coercion, I've just removed caching in write_method
Is't way to go?
If not, maybe we can go back to solution with instance_variable_set to use it here to update part object back, because we usually don't have setters in aggregation part object and can't use them to update object.
Wdyt @josevalim ? Thanks!

@edtsech
Contributor
edtsech commented Jun 19, 2012

Closed by #6743

@edtsech edtsech closed this Jun 19, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment