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

Add after_clone callback #31

Merged
merged 5 commits into from Mar 20, 2019
Merged

Conversation

@elardo
Copy link

@elardo elardo commented Mar 7, 2019

Add after_clone callback to Clowne.

@elardo elardo changed the title [WIP] Add after_clone callback Add after_clone callback Mar 7, 2019
@elardo elardo changed the title Add after_clone callback [WIP] Add after_clone callback Mar 7, 2019
@elardo elardo force-pushed the add-after-clone-callback branch from e591c1d to 9dbb9a4 Mar 7, 2019
@elardo elardo force-pushed the add-after-clone-callback branch from 9dbb9a4 to 8bf2ff6 Mar 9, 2019
@elardo elardo changed the title [WIP] Add after_clone callback Add after_clone callback Mar 9, 2019
@elardo elardo force-pushed the add-after-clone-callback branch from d79a0a8 to 6bf8722 Mar 10, 2019
end

def add_mapping(origin, clone)
@mapper.add(origin, clone)
end

def to_record
@clone
@clone.tap do
run_after_clone
Copy link
Collaborator

@ssnickolay ssnickolay Mar 11, 2019

Looks like It will be executed many times:

ACloner < Clowne::Cloner
  after_clone do |*|
    puts "I'm calling you"
  end
end

operation = ACloner.call(record)
operation.to_record
# => "I'm calling you"
operation.persist
# => "I'm calling you"
3.times { operation.to_record }
# => "I'm calling you"
# => "I'm calling you"
# => "I'm calling you"

If so, we should call after_clone only once. (like you have already implemented for Sequel)

Copy link
Author

@elardo elardo Mar 11, 2019

You were right, it was called multiple times. I added result memoization to Clowne::Utils::Operation#to_record, now after_clone is called only once.

Copy link
Collaborator

@ssnickolay ssnickolay Mar 19, 2019

Nice! Just one last remark - what do you think about avoiding of using tap (because it is redundant):

def to_record
  return @_record if defined?(@_record)
  run_after_clone
  @_record = @clone
end

Or tap because of Rubocop stricts?

Copy link
Author

@elardo elardo Mar 19, 2019

Thank you for remark! I removed redundant tap call, now it looks better.

@ssnickolay
Copy link
Collaborator

@ssnickolay ssnickolay commented Mar 19, 2019

@elardo can you give me your twitter account if you have it?)

@elardo
Copy link
Author

@elardo elardo commented Mar 19, 2019

@ssnickolay ssnickolay merged commit 50cd2e2 into clowne-rb:master Mar 20, 2019
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants