Closed getzze closed 8 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
59472ca
) 99.89% compared to head (c8881b2
) 99.89%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Comparing getzze:emit-old-value
(c8881b2) with main
(59472ca)
❌ 1 (👁 1)
regressions
✅ 65
untouched benchmarks
Benchmark | main |
getzze:emit-old-value |
Change | |
---|---|---|---|---|
👁 | test_create_signal |
125.2 µs | 139.8 µs | -10.44% |
I put the old value in second place because it should allow to connect to slots defined with only the new value as argument. However, in order to not be a disturbing change, it would be better if it is opt-in.
I actually think it should be fine to simply add the second argument. psygnal works fine if the callback takes less (but not more) than the number of arguments in the signal. So, it's always a non-breaking change to pass more arguments to a signal...
The one caveat is that if someone is connecting to the full SignalGroup, and expecting EmissionInfo.args
to be a specific length, then they will have an index error. But I think that's probably a very rare occurrence at the moment (we could github search for it) and well worth avoiding the ugliness of an emit_old_value
opt-in
can you try reducing this PR down to just the change self.signal.emit(new, self._prev)
and seeing if it works for you? While also keeping all of the existing tests passing?
Well, it's true. I will keep it to a minimum then and test it.
Thanks
On 31 January 2024 18:50:17 GMT+00:00, Talley Lambert @.***> wrote:
I put the old value in second place because it should allow to connect to slots defined with only the new value as argument. However, in order to not be a disturbing change, it would be better if it is opt-in.
I actually think it should be find to simply add the second argument. psygnal works fine if the callback takes less (but not more) than the number of arguments in the signal. So, it's always a non-breaking change to pass more arguments to a signal.
can you try reducing this PR down to just the change
self.signal.emit(new, self._prev)
and seeing if it works for you? While also keeping all of the existing tests passing?-- Reply to this email directly or view it on GitHub: https://github.com/pyapp-kit/psygnal/pull/257#issuecomment-1919728118 You are receiving this because you authored the thread.
Message ID: @.***>
Done and I corrected the tests.
thanks! One more change I think we should make:
on line 143 in _group_descriptor.py
, in the function _build_dataclass_signal_group
can you change the following line
signals[name] = Signal(object if type_ is None else type_)
to this:
field_type = object if type_ is None else type_
signals[name] = Signal(field_type, field_type)
That is where we actually construct all of the Signal
instances based on the dataclass to declare the signature of the signal emission, and since we're now emitting two values, we should update that.
want to test out a couple other things locally, but will merge soon. Thanks a lot for your PR!
oh, and also it looks like this does break something downstream in magicgui (which depends on psygnal). glad I run those tests here :) don't feel like you have to figure that one out, i'll also have a look into that.
ok, the issue there is that magicgui makes use of Signal.connect_setattr
which works fine for a single-argument signal, like this:
>>> class Emitter:
... sig = Signal(int)
...
>>> class SomeObj:
... x = 1
...
>>> e = Emitter()
>>> my_obj = SomeObj()
>>> e.sig.connect_setattr(my_obj, 'x')
>>> e.sig.emit(5)
>>> assert my_obj.x == 5
However, (and I think this was probably a poor choice on my part), if the signal emits multiple arguments, it bundles them into a tuple:
>>> class Emitter:
... sig = Signal(int, int)
...
>>> class SomeObj:
... x = 1
...
>>> e = Emitter()
>>> my_obj = SomeObj()
>>> e.sig.connect_setattr(my_obj, 'x')
>>> e.sig.emit(5, 2)
>>> assert my_obj.x == (5, 2)
and that turns out to be a breaking change here... since the behavior of connect_setattr
specifically changes when going from a single value to multi-value signal. I'm sorry, that's totally my bad. Let me ponder that one a little bit. I'd very much like to avoid adding an emit_old_value
argument.
You can force to only accept one argument with e.sig.connect_setattr(my_obj, 'x', maxargs=1)
.
Maybe it could be inforced in the connect_setattr
method, it makes sense that it accepts only one argument by default.
Actually, using max_args=1
when defining a signal.connect
callback is the change that people would have to make to ensure the previous behavior is preserved.
>>> class Emitter:
... sig = Signal(int, int)
...
>>> class SomeObj:
... x = 1
...
>>> e = Emitter()
>>> my_obj = SomeObj()
>>> e.sig.connect_setattr(my_obj, 'x', maxargs=1)
>>> e.sig.emit(5, 2)
>>> assert my_obj.x == 5
connect_setitem
would have the same issue. But again it makes sense to change the default value of the maxargs
argument to 1
.
yep, I agree, that does make sense, and it would be the easiest fix.
But that too is a breaking change for anyone not explicitly passing the maxargs
argument to a signal with multiple arguments, expecting to get all the args, so, I think i would want to put that change behind a FutureWarning
. I'm thinking maybe the best approach is to temporarily patch the Signal
instance specifically used for the evented dataclasses (so that it uses maxargs=1)... and also add the FutureWarning
to the main class, so that we can eventually remove the patched version for dataclasses. it's not pretty, but it's a non-breaking way to undo my mess :) will post tonight
I made a search in github for connect_setitem
and connect_setattr
and the only matches are in magicgui
, microvis
(your packages) and napari
but it seems it's another implementation.
None of the matches define maxargs
, so setting the default maxargs=1
for these two methods is probably a limited breaking change.
But that too is a breaking change for anyone not explicitly passing the
maxargs
argument to a signal with multiple arguments, expecting to get all the args, so, I think i would want to put that change behind aFutureWarning
.
I think people would naturally use connect_setattr
and connect_setitem
with a signal with 1 argument because it is expected to change only one value. So setting maxargs=1
would in fact make more sense than maxargs=None
.
What can be a breaking change is connecting the attribute change signal to a slot with one mandatory argument and one optional. Because now the attribute change signal has 2 values, the second value would be passed to the optional argument, making breaking changes...
So setting maxargs=1 would in fact make more sense than maxargs=None.
totally agree with this. it's less about "sensical" right at the immediate moment, and more about the safest way to get us there
I made a search in github for connect_setitem and connect_setattr and the only matches are in magicgui, microvis (your packages) and napari but it seems it's another implementation.
Yep, I also did the search, and agree, it's unlikely to affect anyone. napari
is currently using an internal event, but our development has been parallel enough (with core devs in both) that it's worth at least maintaining parity. Anyway, call it an overabundance of caution :)
What can be a breaking change is connecting the attribute change signal to a slot with one mandatory argument and one optional.
Yep, good point!
Anyway, please merge in main now that #258 is in, and then add the change mentioned in https://github.com/pyapp-kit/psygnal/pull/257#issuecomment-1920045226 ... and we should be good here
thanks a lot @getzze for this PR and for helping me think through the implications of the change. merging now, and will release in the next day or two as v0.10.0
I'm using
SignalGroupDescriptor
(orevented
) with anattrs
dataclass and I would like the change signal to emit the old value alongside the new value when mutated. It's a minimal change to allow that, in_evented_decorator._changes_emitted
method__exit__
, just change:self.signal.emit(new)
-->self.signal.emit(new, self._prev)
I put the old value in second place because it should allow to connect to slots defined with only the new value as argument.
However, in order to not be a disturbing change, it would be better if it is opt-in.
That is this PR. It is working, but not with subclassing. Any idea?