feat: add collect_fields option to SignalGroupDescriptor, and accept a SignalGroup subclass

[getzze]

To be implemented _build_dataclass_signal_group cannot be cached with lru_cache, so the result is cached in the SignalGroupDescriptor instead.

PR extracted from #265

[codspeed-hq]

CodSpeed Performance Report

Merging #291 will not alter performance

_{Comparing getzze:collect_fields (7db13a7) with main (f37c26f)}

Summary

✅ 66 untouched benchmarks

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (f37c26f) to head (7db13a7).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #291   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         1973      1986   +13     
=========================================
+ Hits          1973      1986   +13     

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

[tlambert03]

very nice. Good to go by me, one very minor comment on camel case naming of the created group class

[getzze]

The PR changes the previous default behavior when a subclass of SignalGroup is provided (now it collects the fields). But this default behavior was not clearly documented so I believe it's fine.

Also, signal_group_class=SignalGroup and collect_fields=False fails without a proper error message and only when accessing foo.events. I was not sure what was best, or raising an error at SignalGroupDescriptor.__init__ or fallback to collect_fields=True with a warning.

[tlambert03]

But this default behavior was not clearly documented so I believe it's fine.

i agree. and i think it's a good improvement thanks.

Also, signal_group_class=SignalGroup and collect_fields=False fails without a proper error message and only when accessing foo.events. I was not sure what was best, or raising an error at SignalGroupDescriptor.init or fallback to collect_fields=True with a warning.

ah yeah, that's probably a pretty easy error to run into, since it only requires setting collect_fields=False (as the other is the default). So I agree, add an exception in the __init__ if collect_fields is False and signal_group_class is SignalGroup

one more thing:

let's make SignalGroupDescriptor a generic now: SignalGroupDescriptor(Generic[GroupType])

I'll make some comments inline

[tlambert03]

actually, it's a bit hard to do inline since you haven't touched all the lines needed here. i'll make a PR to your PR

[getzze]

ok, I will add the early error then.

[tlambert03]

hang on just a minute... PR incoming

[tlambert03]

getzze#1

[tlambert03]

(sorry if that gave merge conflicts) :)

[tlambert03]

ah whoops, looks like I forgot to add from __future__ import annotations in test_group_descriptor

[getzze]

It should be fine now :)

[tlambert03]

arggg... pydantic errors in the test_group_descriptor. I had you add __future__.annotations thinking there wasn't any runtime typing used in that test module, but it looks like there was after all. So i guess we'll need to remove that import at the top, and use Optional[Type[SignalGroup]] instead of type[SignalGroup] | None

[getzze]

ok, I'll make the changes.

[getzze]

mmmh there are other problems with types and coverage. Maybe I'll let you deal with them :), you can modify this PR directly if you don't mind.

[tlambert03]

other problems with types and coverage. Maybe I'll let you deal with them :)

will do! was just noticing that. Going to add a # pragma: no cover and be done with it

[tlambert03]

I reverted the generic stuff, and will deal with it in another PR. The reason is that, as implemented, it makes it unnecessarily ugly to know that the __get__ method of the descriptor returns a subclass of SignalGroup:

@dataclass
class T:
    x: int = 0
    e: ClassVar[SignalGroupDescriptor] = SignalGroupDescriptor()

t = T()
reveal_type(T.e)    # N: Revealed type is "psygnal._group_descriptor.SignalGroupDescriptor[Any]" ... fine
reveal_type(t.e)    # N: Revealed type is "Any" .... BAD

this would work ... but I think it's unnecessarily ugly

@dataclass
class T:
    x: int = 0
    e: ClassVar[SignalGroupDescriptor[SignalGroup]] = SignalGroupDescriptor()