Closed tlambert03 closed 8 months ago
Comparing tlambert03:fix-signal-name-conflict
(2a99cbe) with main
(d3d558d)
⚡ 1
improvements
✅ 65
untouched benchmarks
Benchmark | main |
tlambert03:fix-signal-name-conflict |
Change | |
---|---|---|---|---|
⚡ | test_emit_time[partial_method-2] |
135.8 µs | 122.8 µs | +10.55% |
Follow up on https://github.com/pyapp-kit/psygnal/pull/260#issuecomment-1924467493
This first idea I had would be too complicated to implement I think because we need to mess with __getattribute__
and it's very error prone. So I was thinking that SignalGroup
could be a container class instead.
Positive:
__getitem__
, with events["foo.sig"]
Negative
events.connect
would be longer, events.agg.connect
SignalContainer
, and do not touch SignalGroup
so people could choose to use one or the other.# Rough implementation, I have to figure out the exact layout because of the descriptors
class SignalAggInstance(SignalInstance):
def _slot_relay(self, *args: Any) -> None:
pass
...
class SignalAgg(Signal):
_signal_instance_class: SignalAggInstance
# Container for Signal descriptors
class SignalList:
pass
class SignalContainer:
_agg_: ClassVar[SignalAgg]
_signals_: ClassVar[SignalList]
_uniform_: ClassVar[bool] = False
_signal_aliases_: ClassVar[Mapping[str, str | None]]
def __init_subclass__(
cls,
strict: bool = False,
signal_aliases: Mapping[str, str] = {},
) -> None:
"""Finds all Signal instances on the class and add them to `cls._signals_`."""
all_signals = {}
for k in dir(cls):
v = getattr(cls, k)
if isinstance(v, Signal):
all_signals[k] = v
# delete from dir
delattr(cls, k)
cls._signals_ = type(f"{cls.__name__}List", (SignalList, ), all_signals)
cls._uniform_ = _is_uniform(cls._signals_.values())
if strict and not cls._uniform_:
raise TypeError(
"All Signals in a strict SignalGroup must have the same signature"
)
cls._signal_aliases_ = {**signal_aliases}
# Create aggregated signal
cls._agg_ = Signal(...)
return super().__init_subclass__()
def __init__(self):
self.__agg__attribute_name__ == "agg"
for k, v in self._signal_aliases_.items():
if v == "__agg__":
self.__agg__attribute_name__ == k
break
def get_aggregated_signals(self):
return self._agg_
def get_signal(self, name: str):
return getattr(self._signals_, name, None)
def __getattr__(self, name: str, owner):
sig = self.get_signal(name)
if isinstance(sig, SignalInstance):
return sig
if name == self.__agg__attribute_name__:
return self.get_aggregated_signals()
raise AttributeError
class Events(SignalContainer):
sig1 = Signal(str)
sig2 = Signal(str)
events = Events()
def some_callback(record):
record.signal # the SignalInstance that emitted
record.args # the args that were emitted
events.agg.connect(some_callback)
events.sig1.connect(print)
events.sig2.connect(print)
i can definitely see the merit of deprecating/breaking the pattern of using the SignalGroup
itself as a subclass, and instead requiring a more explicit events.agg.connect
when you specifically want to listen for any event in the group. I think that could be well worth the trouble, so I'm very open to it.
Given the size of the change, I will want to ping some interested parties. I think napari is moving towards fully swapping out their events with psygnal, and we'll want to involve some of them as well.
So, consider me enthused and intrigued, but will need more time to go through these proposals, summarize them, and get opinions. Thanks again for you all your time!
One thing that could be done as a quick fix also is defining the __set__
method for Signal
to make it a data descriptor.
Then it gets precedence over normal attributes. Like this it raises an early error so it's easier to understand the problem and debug:
https://docs.python.org/3/howto/descriptor.html#descriptor-protocol
class Signal:
...
def __set__(self, instance: Any, value: Any) -> None:
"""Define __set__ to make it a data descriptor.
A data descriptor takes precedence over normal attributes with the same name.
"""
raise AttributeError(
f"Setting SignalInstance {self._name!r} to a new value is not allowed.\n"
f"Maybe the signal name conflicts with an attribute of {type(instance)}"
)
...
closing this in favor of #269
alternative to #260, for discussion