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

correctly dup objects #516

Closed
wants to merge 2 commits into from
Closed

Conversation

KaylaGallatin
Copy link

Attempting to fix #355

After calling dup on an object, changes made to either object would be applied to both objects. This fixes that issue so that both objects are independent of one another.

Copy link
Collaborator

@zacharywelch zacharywelch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up @KaylaGallatin 🎩 Had a few comments before merging 😺

def initialize_dup(other)
@_her_attributes = @_her_attributes.dup
@_her_attributes[self.class.primary_key] = nil

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also run_callbacks :initialize here?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also run_callbacks :initialize here?

@_her_attributes = @_her_attributes.dup
@_her_attributes[self.class.primary_key] = nil

@new_record = true
Copy link
Collaborator

@zacharywelch zacharywelch Feb 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can remove this since new_record? doesn't rely on an instance variable yet 😺

def initialize_dup(other)
@_her_attributes = @_her_attributes.dup
@_her_attributes[self.class.primary_key] = nil

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To stick w/ ActiveRecord behavior, we'll also need to clear or assign nil to any association caches.

Associations which aren't included in the initial request are stored in separate instance variables following a @_her_association_<name> convention.

Associations which are part of the initial request are merged w/ @_her_attributes.

@@ -28,6 +28,17 @@ def [](attribute_name)
def singularized_resource_name
self.class.name.split('::').last.tableize.singularize
end

# @private
def initialize_dup(other)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we go ahead and move this to attributes.rb below initialize?

Move to attributes file
Clear association keys and caches
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dup and clone doesn't work on Her::Models
3 participants