api: remove parameters field from Trait #2940
Conversation
superlopuh
requested review from
AntonLydike,
math-fehr,
PapyChacal,
n-io,
webmiche and
dk949
| def get_trait( | ||
| cls, trait: type[OpTraitInvT], parameters: Any = None | ||
| ) -> OpTraitInvT | None: | ||
| def get_trait(cls, trait: type[OpTraitInvT] | OpTraitInvT) -> OpTraitInvT | None: |
There was a problem hiding this comment.
This is a breaking change, but the fix is just a couple of backspaces to delete the comma and instantiate the class instead of passing the parameters separately
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2940 +/- ##
==========================================
- Coverage 89.88% 89.88% -0.01%
==========================================
Files 408 408
Lines 50908 50916 +8
Branches 7880 7884 +4
==========================================
+ Hits 45760 45767 +7
- Misses 3908 3909 +1
Partials 1240 1240 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
LGTM in general.
I am not sure we want to remove parameters from traits. It feels to me like parameters are something that traits should have. Or at least, that the concept of a parametrized trait deserves its own class.
So, to me, this is fine in a technical sense, but it feels to me like what I understand as a trait and what the class implements is not quite the same anymore.
That's not a blocking comment though, I am fine with merging this.
|
Thank you! To me the thing that makes this still have the property of "traits with parameters" is that frozen dataclasses have fields that you can inspect and reason about, so you still have this notion of a named introspectable structure, but without more infrastructure on our end. |
| if not parameters: | ||
| raise ValueError("parameters must not be empty") | ||
| super().__init__(parameters) | ||
| def __init__(self, head_param: type[Operation], *tail_params: type[Operation]): |
There was a problem hiding this comment.
There is no super init here that would set the op types property, as far as I understand
|
|
||
| def __init__(self, head_param: type[Operation], *tail_params: type[Operation]): | ||
| super().__init__((head_param, *tail_params)) | ||
| object.__setattr__(self, "op_types", (head_param, *tail_params)) |
These make the API more complicated than it needs to be, since all traits are frozen dataclasses we can let the dataclass annotation do the heavy lifting for comparison and hashability.