refactor!: New SignalGroup that does not subclass SignalInstance

[tlambert03]

This PR implements the idea proposed by @getzze in #267

Notable changes in this PR:

• SignalGroup is no longer a SignalInstance subclass. All methods that used to be accessible on SignalGroup (such as connect, disconnect, blocked, etc...) should now be accessed on the signal relay at group.all .
• Signal instances may now be accessed on a group using both SignalGroup.__getitem__, in addition to __getattr__.
• SignalGroup is sort of like a Mapping, with methods __iter__ (to list all signal names), __len__ (number of signals), __getitem__ (get a signal). But does not currently subclass abc.Mapping (so it does not have items(), values(), keys())

Deprecations:

• accessing any SignalInstance name (e.g. connect, etc) on a SignalGroup is deprecated, (but still works for now). Using group.all instead.
• accessing signals on a group instance (to get a dict of name->signal instance) is deprecated. Instead use a combination of Mapping methods __iter__ and __getitem__/__getattr__

Breaking:

• Because SignalGroup no longer inherits SignalInstance, the semantics of the __len__ method on a signal group changes; it now returns the number of signal names, not the number of connected slots. This is probably the main breaking change that I can't deprecate.

closes #260
closes #267

[codspeed-hq]

CodSpeed Performance Report

Merging #269 will degrade performances by 23.5%

_{Comparing tlambert03:new-signal-group (dbbd2fa) with main (94d0a67)}

Summary

❌ 5 (👁 5) regressions
✅ 61 untouched benchmarks

Benchmarks breakdown

	Benchmark	main	tlambert03:new-signal-group	Change
👁	test_dataclass_group_create[attrs]	2.4 ms	2.8 ms	-14.79%
👁	test_dataclass_group_create[dataclass]	2.1 ms	2.4 ms	-14.9%
👁	test_dataclass_group_create[msgspec]	2.4 ms	2.8 ms	-11.44%
👁	test_dataclass_group_create[pydantic]	2.4 ms	3.1 ms	-23.5%
👁	test_evented_setattr	37.9 µs	45.2 µs	-16.22%

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (94d0a67) 100.00% compared to head (dbbd2fa) 100.00%.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #269   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         1852      1898   +46     
=========================================
+ Hits          1852      1898   +46     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tlambert03]

@getzze, if you have a moment to take a look, I would appreciate it! This implements your proposal from #262 (comment)

It does not yet add aliases or prefixes as you've done in other PRs (you can do those in another PR if you still want them after this)

[getzze]

Some idea for later (for instance for the nested events): add a method to SignalRelay to merge two SignalRelays:

def update(self, relay: SignalRelay, take_over: bool = True):
    self._signals.update(relay._signals)
    self._sig_was_blocked.update(relay._sig_was_blocked)
    for sig in relay._signals.values():
        if take_over:
            sig.disconnect(relay._slot_relay)
        sig.connect(self._slot_relay, check_nargs=False, check_types=False, unique=True)

[tlambert03]

Also, it would be good to define a function that returns the SignalRelay from a SignalGroup, so we have an easy public API to get the relay, an API that would not change if the private attributes of SignalGroup are redefined for instance. It would be better if it was a method of SignalGroup but the name could confict with signals.

yeah, this is what I was getting at in #269 (comment)

I agree, there should be a guaranteed public property or method. my inclination would be an @property, as the final syntax is nicer, but do you have a preference for a getter method instead?. And the exact name is the main question. Something like .psygnal_relay is pretty guaranteed to never have a name conflict.
And if we do that, is there perhaps no reason to provide all the logic for letting them override the public name? Maybe one safe public name is all we need

[getzze]

I was proposing functions and not methods to avoid any name conflict. But if the method name starts with psygnal and is long, it's very unlikely that it will conflict with a dataclass field. And a method is more convenient because of auto-completion.

The good thing about all is that it's short. Maybe it's not worth putting all the code for allowing changing the all name though, and just have a long property/method name. If it's documented what the possible conflicts are and there are very few and unlikely names it should be ok.

We can always add possibility to use aliases, which are useful even after this PR, to alias a private field to a public name.

[tlambert03]

thanks @getzze! yeah I agree. feels tough to make this rather tiny-but-important decision. I might ping @jni for his thoughts on this:

Juan, we all like the idea of events.all.connect ... but all still has the possibility of name conflict (as you pointed out before). The current PR allows the user to change the name all to something else if they want to use it for themselves (thereby avoiding the conflict) ... but then we have the problem of a third party wanting to know how to get at the aggregate/relay connector. That leads to a desire to have a permanent public name like events.all_psygnals.connect or events.get_relay().connect, which are uglier, to be sure. What do you think? some options:

1. stick with events.all by default, but allow the user to pick their own public name, and to hell with anyone else who receives the object 😂 (this is probably a no... since magicgui for example needs to know how to get at the relay)
2. keep events.all, and allow user-overrides, and add a permanent safe public name like events.all_psygnals (name suggestions welcome)
3. use events.all, and if someone has a dataclass with a field named all, then they must access it using group['all']. because events.all will always point to the relay.
4. don't use events.all, just go with one permanent ugly public name events.all_psygnals that's unlikely to ever conflict with a dataclass field.

[getzze]

I think I like 3 better. Somebody mentioned the example of `pandas` that I think is a good model: you can access ALL the columns by key, like `events["name"]`, and by attribute, only the columns with a well-behaved (no space) and non-conflicting name.

…

On 15 February 2024 21:43:16 GMT+00:00, Talley Lambert ***@***.***> wrote: thanks @getzze! yeah I agree. feels tough to make this rather tiny-but-important decision. I might ping @jni for his thoughts on this: Juan, we all like the idea of `events.all.connect` ... but `all` still has the possibility of name conflict (as you pointed out before). The current PR allows the user to change the name `all` to something else if they want to use it for themselves (thereby avoiding the conflict) ... but then we have the problem of a third party wanting to know how to get at the aggregate/relay connector. That leads to a desire to have a permanent public name like `events.all_psygnals.connect` or `events.get_relay().connect`, which are uglier, to be sure. What do you think? some options: 1. stick with `events.all` by default, but allow the user to pick their own public name, and to hell with anyone else who receives the object 😂 _(this is probably a no... since magicgui for example needs to know how to get at the relay)_ 2. keep `events.all`, and allow user-overrides, *and* add a permanent safe public name like `events.all_psygnals` (name suggestions welcome) 3. use `events.all`, and if someone has a dataclass with a field named `all`, then they *must* access it using `group['all']`. because `events.all` will always point to the relay. 4. don't use `events.all`, just go with one permanent ugly public name `events.all_psygnals` that's unlikely to ever conflict with a dataclass field. -- Reply to this email directly or view it on GitHub: #269 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>

[tlambert03]

you can access ALL the columns by key, like events["name"], and by attribute, only the columns with a well-behaved (no space) and non-conflicting name.

yep, and combined with the warning at class creation that you stubbed out in #269 (review) ... i think there will be no surprises

[jni]

I like 3 also, by a fair margin, so I am happy that we are all in agreement. 😊

[tlambert03]

implemented in 47bf3ae

[tlambert03]

ok! this one is going in :)

Thanks all for the helpful discussion. Huge thanks to @getzze for getting this name-conflict discussion started in #260, and for proposing the solution that was ultimately implemented here in #262 (comment). It's not often that someone comes along and dives deep enough to understand the existing codebase and provides very insightful and productive suggestions. I hope you don't mind that I ran with that idea and implemented it here (it was a big change that I wanted to consider carefully), and I hope that you'll continue contributing improvements :)

[tlambert03]

this will be released as v0.10.0rc0 for a week or so before release

[getzze]

Ahahah, likewise, it's not every time that developers include contributors, who know less about the project, to make big changes. I have others PRs to propose ;)

…

On 16 February 2024 14:54:46 GMT+00:00, Talley Lambert ***@***.***> wrote: ok! this one is going in :) Thanks all for the helpful discussion. Huge thanks to @getzze for getting this name-conflict discussion started in #260, and for proposing the solution that was ultimately implemented here in #262 (comment). It's not often that someone comes along and dives deep enough to understand the existing codebase and provides very insightful and productive suggestions. I hope you don't mind that I ran with that idea and implemented it here (it was a big change that I wanted to consider carefully), and I hope that you'll continue contributing improvements :) -- Reply to this email directly or view it on GitHub: #269 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>