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 support for generic signals #357

Merged
merged 10 commits into from Oct 30, 2014

Conversation

Projects
None yet
2 participants
@ErwinJunge
Copy link
Contributor

ErwinJunge commented Sep 24, 2014

This is achieved by adding a keyword (signals) to register. This accepts
a list-like item containing signals to connect to. Some special-casing
was added to handle pre_delete. The default is still to just connect to post_save.

ErwinJunge added some commits Sep 24, 2014

Add support for generic signals
This is achieved by adding a keyword (signals) to register. This accepts
a list-like item containing signals to connect to. Some special-casing
was added to handle pre_delete. The default is still to just connect to post_save.
@etianen

This comment has been minimized.

Copy link
Owner

etianen commented Sep 26, 2014

Hi,

Thanks for contributing! Could you explain what the intended use-case for this is?

I'm a little bit concerned about the special-casing for the pre_delete signal. Presumably there are other custom signals that might need this special casing too?

Essentially, most some signals need deferred handling, but some (pre_delete), need eager handling. I wonder if there is a nice syntax we could use for specifying this when adding custom signals?

@ErwinJunge

This comment has been minimized.

Copy link
Contributor

ErwinJunge commented Sep 26, 2014

The intended use-case (which we're already using internally and I'm hoping to publish as soon as time permits) is a soft-delete solution built on top of django-reversion.

post_save is not the right signal for this for two reasons:

  1. In case of cascading deletes we might lose information on child objects
  2. The table gets "polluted" with a lot of versioning information which is uninteresting for the soft-delete usecase

To elaborate on point 1. (the most significant point), consider the following flow:

  1. A Chapter object is created (triggers post_save on Chapter object)
  2. A Page object (which is a child of the Chapter) is created (triggers post_save on Page object, but not on Chapter)
  3. Chapter object is deleted (does not trigger post_save, so no handling by django-reversion)
    3a. Due to cascading deletes, the Page object is also deleted
  4. The Chapter object is restored, based on the latest information in django-reversion. Since this is the information stored at time of 1, the Page object is not restored.

By storing the information on pre_delete, we can ensure that all the information neccesary for a full restore is available.

Regarding the special-casing, I agree that there might be other signals (especially user-defined ones) that might need eager handling as well. We could pass in a list of dictionaries with 'signal' and 'eager' instead of just a list of signals and store whether a signal should be processed as eager in an attribute on the revision manager. I'll augment the pull request to facilitate this.

@etianen

This comment has been minimized.

Copy link
Owner

etianen commented Sep 29, 2014

Thanks for the comprehensive response... I can see how this would be useful. Apologies for taking a few days to get back to you on this. I'm afraid that I'm just come back from a camping trip.

I'm not so sure about the dictionary syntax for the signals. It seems clumsy to use strings like this. Two ideas present themselves:

reversion.register(SomeModel, signals={post_save: False, pre_delete: True})

Or:

reversion.register(SomeModel, signals=(post_save,), eager_signals=(pre_delete,))

I'd tend to prefer the second, as it's more explicit as to what is being done.

@ErwinJunge

This comment has been minimized.

Copy link
Contributor

ErwinJunge commented Sep 29, 2014

Good idea. I have some other stuff to attend to right now, but I'll get a pull-request to you later today.

ErwinJunge added some commits Sep 29, 2014

@ErwinJunge

This comment has been minimized.

Copy link
Contributor

ErwinJunge commented Oct 27, 2014

Ping :) Are there issues that you'd like to have resolved before this can be merged?

@@ -398,14 +407,18 @@ def get_adapter(self, model):
model = model,
))

def unregister(self, model):
def unregister(self, model, signals=None):

This comment has been minimized.

@etianen

etianen Oct 29, 2014

Owner

It feels like unregister() should unregister from all the signals provided in register(), rather than allowing selective unregistration. I'd suggest also storing self._signals, in the same way as you're storing self._eager_signals, and looping over both of these in unregister.

# Connect to the post save signal of the model.
post_save.connect(self._post_save_receiver, model)
# Connect to the selected signals of the model.
all_signals = list(signals or []) + list(eager_signals or [])

This comment has been minimized.

@etianen

etianen Oct 29, 2014

Owner

This can just be list(self._signals) + list(self._eager_signals), if below comment is implemented.

@etianen

This comment has been minimized.

Copy link
Owner

etianen commented Oct 29, 2014

Sorry, I don't actually get notified when new code goes up. It just appears silently. :S

I've made some comments on the changes. I wonder, would it also be possible to get some tests in?

@ErwinJunge

This comment has been minimized.

Copy link
Contributor

ErwinJunge commented Oct 30, 2014

Good suggestions, implemented.

I also added some tests specifically for the pre_delete functionality, but I'm not sure if those are sufficient. You have a rather comprehensive set of tests :) If you could point me at anything specific you'd like me to test I'd be happy to add some more tests.

@etianen

This comment has been minimized.

Copy link
Owner

etianen commented Oct 30, 2014

Thanks, this looks excellent!

@etianen etianen closed this Oct 30, 2014

@etianen etianen reopened this Oct 30, 2014

etianen added a commit that referenced this pull request Oct 30, 2014

@etianen etianen merged commit 2b40908 into etianen:master Oct 30, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details
@etianen

This comment has been minimized.

Copy link
Owner

etianen commented Oct 30, 2014

(At some point, I'll integrate a code coverage report on Travis, but at the moment, it's good that the feature is at least partly tested for the easy cases).

@ErwinJunge ErwinJunge deleted the ErwinJunge:generic-signals-support branch Oct 30, 2014

@ErwinJunge

This comment has been minimized.

Copy link
Contributor

ErwinJunge commented Oct 30, 2014

Great :) As one final question: in what timeframe do you expect this to surface in a stable release on pypi? When I have spare time I'm planning on extracting the undelete feature I built out into a seperate package so that I can release it and I don't want it to rely on unreleased code :) No hurry though, just wondering.

@etianen

This comment has been minimized.

Copy link
Owner

etianen commented Oct 30, 2014

I can push out a release tomorrow, no worries!

On 30 Oct 2014, at 14:09, Erwin Junge notifications@github.com wrote:

Great :) As one final question: in what timeframe do you expect this to surface in a stable release on pypi? When I have spare time I'm planning on extracting the undelete feature I built out into a seperate package so that I can release it and I don't want it to rely on unreleased code :) No hurry though, just wondering.


Reply to this email directly or view it on GitHub.

@etianen

This comment has been minimized.

Copy link
Owner

etianen commented Oct 31, 2014

Tadah!

https://pypi.python.org/pypi/django-reversion/1.8.5

On 30 Oct 2014, at 14:18, Dave Hall dave@etianen.com wrote:

I can push out a release tomorrow, no worries!

On 30 Oct 2014, at 14:09, Erwin Junge notifications@github.com wrote:

Great :) As one final question: in what timeframe do you expect this to surface in a stable release on pypi? When I have spare time I'm planning on extracting the undelete feature I built out into a seperate package so that I can release it and I don't want it to rely on unreleased code :) No hurry though, just wondering.


Reply to this email directly or view it on GitHub.

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