Open getzze opened 8 months ago
Attention: Patch coverage is 92.56198%
with 9 lines
in your changes are missing coverage. Please review.
Project coverage is 99.57%. Comparing base (
f6cebcb
) to head (f5d4fa4
).
Files | Patch % | Lines |
---|---|---|
src/psygnal/_dataclass_utils.py | 91.00% | 9 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Comparing getzze:alias
(f5d4fa4) with main
(f6cebcb)
❌ 4
regressions
✅ 62
untouched benchmarks
:warning: Please fix the performance issues or acknowledge them on CodSpeed.
Benchmark | main |
getzze:alias |
Change | |
---|---|---|---|---|
❌ | test_dataclass_group_create[attrs] |
2.3 ms | 2.6 ms | -10.21% |
❌ | test_dataclass_group_create[dataclass] |
2 ms | 2.3 ms | -11.31% |
❌ | test_dataclass_group_create[msgspec] |
2.4 ms | 2.7 ms | -12.51% |
❌ | test_dataclass_group_create[pydantic] |
2.3 ms | 3.1 ms | -24.23% |
hey @getzze, thanks again for all your work on this. it's greatly appreciated.
I took a quick look yesterday and I like a lot of what I see, and also have a couple minor reservations that I want to think through a bit more (such establishing any new patterns for field-specific metadata, such as the Annotated pattern for pydantic2). But in general, I think you're right: it's a very flexible solution that solves most of the issues you've pointed out so far.
One other thing I'll mention is the library fieldz. I have a couple other libraries where I want to be able to support a bunch of different dataclass-like patterns, and fieldz grew out of that. I hadn't used it yet here to avoid an additional dependency, but with this PR, it might be time to consider using fieldz.fields()
instead of the the iter_fields()
method that i had, and which you've extended in this PR. (it's possible we'll need to make a PR or two to fieldz to make sure we're grabbing field-specific metadata in a way that works for psygnal too)
it's been a busy week here, so sorry that I haven't commented yet, but will have a deeper look soon. (In the meantime, don't worry too much about the mypyc compiled tests failing)
Now it fails only with compile=True
, so I'm done :)
I didn't find any official way to add metadata to fields in Pydantic v2, but in the forums people seem to pass the metadata as a dict in the second argument of Annotated
. It works but this mechanism was not thought to be used like that indeed: https://docs.pydantic.dev/latest/api/fields/#pydantic.fields.FieldInfo
It would be great to add the iter_fields_with_options
function to fieldz
, although as you said that would be an extra dependency.
Sorry for the large PR, it has all the part about metadata for individual fields but also the part about signal aliasing.
The signal aliasing mechanism could help solve #260 and #262 if SignalGroup
is redesigned as a container with a Signal proxy attribute and a list of attached Signals. I'll explain better in #262
I rebased to main
. I just realized that one part of this PR should have gone to v0.10
release, I can make a separate PR ([EDIT] #289 ).
Do you still think it makes sense to make SignalGroup
a Mapping
?
About this PR, I restructured it to include 3 features:
Maybe 3 is not really needed actually.
There is two ways of doing that:
@evented
or SignalGroupDescriptor
The metadata part can go to another PR (or even to fieldz
as you already mentioned).
Another change brought by this PR is the collect_fields
argument to SignalGroupDescriptor
. Before, if you specified a signal_group_class
to a SignalGroupDescriptor
, the fields were not added to it. So you could not provide special signal names, not related to the fields of the dataclass, and at the same time automatically create the signals attached to the fields. With this PR, signal_group_class
defines the base SignalGroup
type (by default it is just SignalGroup
) and specify if you want to also collect the fields with collect_fields
. This could go in another PR ([EDIT] #291 ).
Tell me what you think about it when you have time :slightly_smiling_face:
Once #291 is merged, I'll split this PR, first add the signal_aliases
option, then add the metadata on the fields.
Once #291 is merged, I'll split this PR, first add the
signal_aliases
option, then add the metadata on the fields.
sounds good! the metadata on the fields is the one i'm the most anxious about :) but we'll find something that works
sounds good! the metadata on the fields is the one i'm the most anxious about :) but we'll find something that works
Yes, I realized mypyc was not happy about the metadatas, so it will require more work.
With #299 merged, this PR adds parsing of field metadata to define aliases directly in the fields declarations.
I don't know if it's worth adding, maybe it's too much work to maintain (like keep up with the dataclasses
-like librairies API changes). At least it should be low priority.
It also needs some work to make it work with the compiled version and to improve the speed.
thanks for the update here @getzze, I do think i'm going to take you up on the offer to allow this to be low priority :)
i like the general idea... and indeed there are parts of the EventedModel config that would be better off as a Field()
metadata rather than model metadata.
solves #261 supersedes #260
Add a keyword argument to
SignalGroupDescriptor
to specify signal aliases (if the alias is None, no signal is emitted when changing the field). The signal aliases table is stored in theSignalGroup
class.Also allow for specifying per-field options for Signals using the metadata of the fields (different implementation in the different packages).
It solves #261 elegantly and makes subclassing more automatic: