-
-
Notifications
You must be signed in to change notification settings - Fork 385
Option to "partially override" an attribute from a parent class? #637
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
I do know about Moose and it has been on my agenda to look for inspirations once I run out of own tasks…which weirdly hasn't happened yet. 🤪 Before changing APIs, you can access the field definitions while declaring fields. Which means that the following works: @attr.s
class Two(One):
myattrib = attr.ib(
default=2,
validator=attr.fields(One).myattrib.validator,
converter=attr.fields(One).myattrib.converter,
) Typing aside, writing a helper that gives you something like |
Another detailed description in #698, which is a duplicate. |
So I guess I could be convinced to add something like @attr.define
class Two(One):
myattrib = attr.field_evolve(default=2) That would also keep myattrib in the same order which would solve #707. However:
|
Over at #829 we've come up with a somewhat clunky but magic-free approach of allowing to evolve Attributes (already in) and then convert them to fields/attr.ibs. In your case that would look like this: @attr.define
class Two(One):
myattrib = attr.fields(One).myattrib.evolve(default=2).to_field() I do realize it's a tad verbose, but it's a lot clearer with less indirection than what was proposed so far and would only require us to impolement |
Great. Many thanks for the follow up! I'll check it out! |
To confirm, the above does not exist in the current version yet, correct? |
correct, it does not. |
The above would be great to have - not only for modifying fields of sub-classes but also for "de-duplicating defaults" - when passing arguments down through multiple levels, as in #876. Not sure how possible this is with regard to attrs internals, but from an external perspective it would be seem more intuitive to have the syntax:
|
Re #829 Here's what I'm doing... from attr import define, field, fields, validators, make_class
@define
class One:
myattrib: int = field(
default=1,
validator=validators.instance_of(int),
converter=int,
)
def create_subclass(name, base, **kwargs):
def gen_fields(*args):
for field in args:
if field.name in kwargs:
yield field.evolve(default=kwargs[field.name])
else:
yield field
def reset_defaults(cls, fields):
return list(gen_fields(*fields))
return make_class(name, {}, bases=(base,), field_transformer=reset_defaults)
Two = create_subclass('Two', One, myattrib=2) In [31]: fields(One)
Out[31]: (Attribute(name='myattrib', default=1, validator=<instance_of validator for type <class 'int'>>, repr=True, eq=True, eq_key=None, order=True, order_key=None, hash=None, init=True, metadata=mappingproxy({}), type=<class 'int'>, converter=<class 'int'>, kw_only=False, inherited=False, on_setattr=None))
In [32]: fields(Two)
Out[32]: (Attribute(name='myattrib', default=2, validator=<instance_of validator for type <class 'int'>>, repr=True, eq=True, eq_key=None, order=True, order_key=None, hash=None, init=True, metadata=mappingproxy({}), type=<class 'int'>, converter=<class 'int'>, kw_only=False, inherited=True, on_setattr=None)) |
a quick note to myself, that the implementation needs to take into account |
I've implemented @define
class A:
x: int = 0
y: int = 1
# attrs order: x, y
@define
class B(A):
x: int = evolve_field(default=42)
# attrs order: x, y
@define
class C(A):
x: int = 42
# attrs order: y, x Notice how if you simply redefine the Another issue that came up during my implementation was that some sanity checks like "no attrs without a default following an attr with a default" happen before the field transformer is run, so the field transformer has no chance to fix the issues. You may not run into this since it may not be implemented using a field transformer like I did. |
@jamesmurphy-mc how were you able to implement this? My use-case is similar to yours. Preferably, I would like to implement something like what you have or pass a new field definition altogether that can overwrite the existing field while maintaining order. @define(kw_only=True)
class A:
x: int = field(default=0)
y: str = field(default="1")
z: str = field(default="2")
# attrs order: x (int), y (str), z (str)
# using your approach
@define(kw_only=True)
class B(A):
x: str = evolve_field(type=str, default="0")
# attrs order: x (str), y (str), z (str)
@define(kw_only=True)
class C(A):
x: str = field(default="0")
# attrs order: x (str), y (str), z (str) |
It was a mark and sweep approach, using Just like |
Changing the type of an attribute in a subclass is very tricky. Changing the type to something completely incompatible (like str to int) should never be allowed since it breaks the Liskov substitution principle. Changing the type of a field to a subtype (like changing int to Literal[1]) should only be allowed for frozen classes. Defaults, aliases and kw-only status should be safe since constructors don't obey the LSP. You could argue for a consenting adults approach where we allow anything at runtime, but apply restrictions in Mypy. Still seems like a footgun. |
Hey @Tinche, fully get the LSP argument for I agree that there is probably no obvious answer to this entire Specifically, how would you avoid a situation like from attrs import define
@define
class BaseSettings: ...
@define
class SpecialSettings(BaseSettings): ...
@define
class Base:
settings: BaseSettings
@define
class Special(Base):
settings: SpecialSettings I guess one option is use |
Sure. Imagine this: @define
class ClassA:
a: int Now, imagine a very simple and innocent function, like this: def innocent(a: ClassA) -> None:
a.a += 1 All good so far, right? Now for the final act of the show, imagine: @define
class ClassB(ClassA):
a: Literal[1]
innocent(ClassB(1)) It's a variance issue, the same as with |
Thanks @Tinche, much appreciated. Makes sense, indeed. Still, any thoughts on my example above? What's your take on it, i.e. how would you avoid having to copy-paste the entire field definition in the subclass just to change the type annotation? |
Try something like this? from attrs import define
@define
class BaseSettings: ...
@define
class SpecialSettings(BaseSettings): ...
@define
class Base[T = BaseSettings]:
settings: T
Special = Base[SpecialSettings] |
Yeah, already thought about using lst: list[Base] = [Base(BaseSettings()), Special(SpecialSettings())] since the default type would be used, which gives: error: List item 1 has incompatible type "Base[SpecialSettings]"; expected "Base[BaseSettings]" [list-item] Of course, one could omit then default from the class definition but then ... well ... there would be no default 🙃 So what is the solution here to enable both: a default + a correct list type? |
Since lst: list[Base] = [Base(BaseSettings()), Base(SpecialSettings())] ? |
Doesn't work unfortunately because – and I forget to mention this – @define
class Special(Base[SpecialSettings]):
special_attribute: int = 0 |
Hi,
I'm part of a team that's in the early stages of migrating a large legacy app to Python. I'm pushing pretty hard for the team to adopt attrs as our standard way of doing OOP. I know the "attr way" is to avoid inheritance as much as possible, but that's not always possible, especially when you are part of a team with a project that likes to rely on inheritance (generally nothing too crazy, just simple stuff, but still - it's used a lot in the app and it's a good fit a lot of how the app works). One thing we do a lot is have base classes that device attributes that subclasses "further refine". With these, we want to be able to "extended" the attribute definition from the parent classes vs. totally overwrite it. It doesn't seem like this is possible today in attrs. For example:
Output when run:
So if we want to extend the definition of
myattrib
, we would have to copy/paste the full definition vs. just modify what is different. What would people think about an option that says "take the full definition from the superclasses and just "merge in" this one extra aspect." Having to repeat the full definition across an inheritance hierarchy gets old (and ugly) pretty quick. :-)Just by way of context on both this ticket and the other ticket I have opened (#573 - lazy=True option), the legacy app we are using is coming from Perl. Yes, I know everyone loves to hate Perl, :-) but hear me out. :-) The app uses Moose (https://metacpan.org/pod/Moose) and it's "lightweight cousin" Moo (https://metacpan.org/pod/Moo) and for all the bad things everyone wants to cast at Perl, the Moose/Moo "OOP framework" is really nice and has some great features. It has completely changed how OOP is done in Perl and took it from horrible to really nice (nobody has done OOP in Perl for 10+ years without using Moose.) (The creator of Moose, Stevan Little, spent lots of time studying and using lots of other OOP methodologies and was able to incorporate the "best of the best" ideas.) It's very similar to attrs in many ways, but there are a few things missing that are SOOO handy, useful, powerful, etc. :-)
In terms of how Moose would write the above example (and use the "+" to signify an "override"):
Would something like an
extend=True
option to attrs be a nice addition to trigger similar behavior to the "+" shown above?Thank you
The text was updated successfully, but these errors were encountered: