Proposal for alternate SignalGroup pattern #267
Description
This is meta-issue collecting thoughts and proposals about solving name conflicts in SignalGroups.
(see also)
- Name conflict in SignalGroup #260
- Specify the name of a single changed signal in EventedModel #261
- fix: avoid name conflicts (option 2) use __get__ directly to create signal instances #262
- feat: signal alias in field metadata #265
The primary problem stems from the fact that
SignalGroupinherits fromSignalInstance(which has a fair number of attributes, likename, etc...)- Signals names within a
SignalGroupare determined by the user, and accessible as attributes - This leads to the possibility of naming collisions (such as
SignalGroup().name)
As a little history, the pattern of inheritance was originally borrowed/inspired from vispy's EmitterGroup (akin to SignalGroup, which inherits from EventEmitter (akin to SignalInstance). I'll note that vispy simply forbids you to use a name that conflicts. But I don't think that's an acceptable approach for psygnal, particularly if we want to support evented dataclasses (which are almost certainly going to have fields like name at some point).
At the moment, I'm leaning towards a new class entirely, as proposed by @getzze in #262 (comment) and copied below. This new class would be a plain container with minimal private attributes, preventing name conflicts. The primary thing we lose there by not inheriting from SignalInstance is that you can't connect to the group class itself using, for example events.connect... but that's easily solveable by either adding an aggregating signal (e.g. events.agg.connect()), or a special method (e.g. events.connect_all() ... risking name conflicts)
Originally posted by @getzze in #262 (comment)
I was thinking that SignalGroup could be a container class instead.
Positive:
- it would really minimize the name conflicts, we could even track the potential conflicts and raise an error so it would be completely conflict-free!
- this redesign would make it easier to call nested signals, e.g. defining
__getitem__, withevents["foo.sig"]
Negative
- writing
events.connectwould be longer,events.agg.connect - it would need a big rewrite, although we could create a new class, for instance
SignalContainer, and do not touchSignalGroupso 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)