SignaledDataset _on_delete signal sent before actual deletion

[mrvisscher]

In the SignaledDataset class

brightway2-data/bw2data/signals.py

Lines 243 to 259 in f9f3663

	class SignaledDataset(Model):
	@override
	def save(self, signal: bool = True, *args, **kwargs) -> None:
	"""Receives a mapper to convert the data to the expected dictionary format"""
	old = type(self).get_or_none(type(self).id == self.id)
	super().save(*args, **kwargs)
	if signal:
	signaleddataset_on_save.send(
	old=old,
	new=self,
	)
	@override
	def delete_instance(self, signal: bool = True, *args, **kwargs) -> None:
	if signal:
	signaleddataset_on_delete.send(old=self)
	super().delete_instance(*args, **kwargs)

The save signal is sent after calling save on the superclass. However, this is not the case for the delete signal. Which is sent before the dataset is actually deleted. Meaning that connected code that expects the dataset to not be there anymore fails.

It makes most sense to me that the delete signal is sent after deletion, so connected views can adjust to the new model accordingly. In any case the implementation of the save and delete signals is not consistent right now. With the first being send before model mutation and the latter being send after.

[cmutel]

Thanks @mrvisscher

This behaviour is intentional, but I understand that it might not be obivous. The point here is that calling after then actual deletion would mean that consumers no longer have access to the object which was deleted, so they can't examine its attributes and make whatever changes they need.

@jsvgoncalves Maybe it would make more sense to call this pre_delete?

[jsvgoncalves]

The three options are:

i. change signal name, which implies changing downstream code that already connects to this code
ii. change signal trigger to after the delete, which implies changing downstream code that relies on the signal being triggered before
iii. change nothing as this was intended and document behaviour

brightway2-data/bw2data/signals.py

Line 24 in f9f3663

doc="""Emitted *before* a SignaledDataset is deleted.

While I understand the name might can be slightly misleading at first, I would have a slight preference for iii., as the two other options are a breaking change to the existing API and cascade downstream to clients of the API.

[mrvisscher]

Fair enough.

It would be very useful to have an after_delete signal at the very least. So our views can update with the new model change, when it has actually changed.

[jsvgoncalves]

It would be very useful to have an after_delete signal at the very least.

That makes sense to me and a good argument in favour of:

i. change signal name, which implies changing downstream code that already connects to this code

plus adding a "new signal" that's consistent with the behaviour of on_save.