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

Fix clear cache for delete db #216

Closed
wants to merge 2 commits into from
Closed

Conversation

MarkFull
Copy link

@MarkFull MarkFull commented Oct 11, 2016

The current way of clearing cache when deleting db does not work well.
The reason is include does not override the delete! method.

In this way this PR is fixing the problem: reopen the class, alias_method delete! to another name, clear cache in the redefined method, to achieve the overridden effect.

@ktaragorn
Copy link

I work with @MarkFull On further investigation we found that the couchrest_model way of monkeypatching delete! by defining the method in a different module and then including was not taking effect. So potentially that is why this PR was needed, and also why this PR is probably broken. If the delete! is monkey patched by reopening the class instead, then it should work.

Copy link

@ktaragorn ktaragorn left a comment

Choose a reason for hiding this comment

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

Looks like recreate! calls delete! so this change shouldnt be needed. From some debugging looks like the new version of delete! here doesnt seem to take hold.

@ktaragorn
Copy link

The problem with include is that methods of a class cannot be overridden by modules included in that class 

http://stackoverflow.com/a/5944385/1520364

As `include` won't override the original method, reopen the class and `alias_method` did the job. Thanks to help from @ktaragorn
Copy link

@ktaragorn ktaragorn left a comment

Choose a reason for hiding this comment

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

This version of the changes were tested to work with recreate! as well.

@MarkFull MarkFull changed the title Clear cache for recreate db Fix clear cache for delete db Oct 12, 2016
@samlown
Copy link
Member

samlown commented Nov 26, 2016

@MarkFull @kanterov thanks for looking at this! There are a couple of points however. It would be great to have a test demonstrating that this is not currently working as expected, and secondly, given that we no longer support Ruby < 2.0, we can safely use the Module#prepend method. I'll see if I can add these quickly.

@samlown
Copy link
Member

samlown commented Nov 26, 2016

Confirmed and fixed in this commit: 38bce62 Thanks!

@samlown samlown closed this Nov 26, 2016
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.

3 participants