Skip to content
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

[RFC] Inconvenient defaults? #487

Closed
hynek opened this issue Jan 20, 2019 · 53 comments · Fixed by #887
Closed

[RFC] Inconvenient defaults? #487

hynek opened this issue Jan 20, 2019 · 53 comments · Fixed by #887
Labels
Feature Thinking Needs more braining.
Milestone

Comments

@hynek
Copy link
Member

hynek commented Jan 20, 2019

As part of “Projectimport attrs” (see also #408) we get a unique opportunity to change default options/behaviors that grew over the years but couldn't be fixed due to backward-compatibility restrictions.

The rough plan is create a new, friendlier API on top of @attr.s and attr.ib() that won't go anywhere.

The following is cast in stone:

The following would be really nice to have:

What else is bugging you?


One thing that I just can't make up my mind is related to #223: should we make slots=True the default? I’m quite confident that in 99,9% of cases it's the right thing to do and will guide people to write better classes.

However on the other hand, our approach of rewriting classes breaks in certain scenarios, usually involving metaclasses.

So the question is, whether we want to tolerate a higher rate of bogus bug reports/help requests or make the default case nicer?

I welcome your input.


Finally a controversial idea: we could make import attrs Python 3 only. There isn't much baggage we'd get rid of but there is some and 2020 is less than a year ahead. It would also allow us to embrace enums as part of our API.

@sfermigier
Copy link

"we could make import attrs Python 3 only." -> +1.

"It would also allow us to embrace enums as part of our API." -> That would be awesome.

@berislavlopac
Copy link

I absolutely agree with your point on slots=True, but I would recommend not making it the default. I'm always surprised by how many people have no idea that this amazing feature exists; you need to at the very least explain in the documentation what it is anyway, so it can be used to recommend its use and guide people to write better classes. 🙂

I'm also in favour of Python3-only import attrs.

What else is bugging you?

A question: My main issue with attrs has always been its decorator-based syntax and inability to instantiate using __init__. To be clear, it's my personal preference and not a general opinion on How Things Should Be Done [TM], but I've always liked the subclassing approach found in e.g. schematics. Also, I've often explained the concept of data classes (regardless of the implementation) as "ORM without the R", as it maps nicely with people's experience with Django and others. So I just have to ask, it there a possibility to have an optional method of implementing attrs as a parent/abstract class and/or a mixin?

Thanks for all the amazing work!

@hynek
Copy link
Member Author

hynek commented Jan 20, 2019

So I just have to ask, it there a possibility to have an optional method of implementing attrs as a parent/abstract class and/or a mixin?

No, but you could probably use metaclasses and our programmatic interface (make_class) to build it yourself?

@ncoghlan
Copy link

In a world of AWS Lambda and other RAM-second based billing systems, making "slots=True" the default will literally save people money (since it uses less RAM, and avoids pointless dict creation for each instance).

If it fails, then folks are likely to get noisy exceptions that a few asked-and-answered questions on Stack Overflow will teach them to resolve with "slots=False".

@bskinn
Copy link

bskinn commented Jan 20, 2019

If it fails, then folks are likely to get noisy exceptions that a few asked-and-answered questions on Stack Overflow will teach them to resolve with "slots=False".

Could even seed SO with a couple of self-answered Q&A covering common error cases where slots=False is the fix.

@hynek
Copy link
Member Author

hynek commented Jan 20, 2019

If it fails, then folks are likely to get noisy exceptions that a few asked-and-answered questions on Stack Overflow will teach them to resolve with "slots=False".

I tend to agree; it just goes a bit against attrs’s principle of “just adding some stuff” to your classes.

@hynek hynek added the Thinking Needs more braining. label Jan 20, 2019
@jml
Copy link

jml commented Jan 20, 2019

My feelpinion: frozen=True should be the default. Probably the option should be renamed to mutable, so you say mutable=True for when you know what you are doing.

@bskinn
Copy link

bskinn commented Jan 20, 2019

My feelpinion: frozen=True should be the default. Probably the option should be renamed to mutable, so you say mutable=True for when you know what you are doing.

Disagree here. frozen=True only works for me in situations where (1) the entire contents of all instances will be known at instantiation-time in all cases, and (2) where no derived/calculated members will be needed (i.e., where @propertys are sufficient for all derived members because the calculations are fast).

I use attrs classes a lot in my data analysis work, and often I'll have something expensive I need to do with the arguments passed in at initialization. I absolutely want to do that expensive thing exactly once for each instance, and AFAIK I have to do it as something like:

@attr.s(slots=True)
class Foo:

    input_var = attr.ib()
    expensive_derived_var = attr.ib(init=False)

    def __attrs_post_init__(self):
        self.expensive_derived_var = self.calculate_expensive_thing()

    def calculate_expensive_thing(self):
        ...
        {expensive stuff with self.input_var}
        ...

With frozen=True, I can't set self.expensive_derived_var in __attrs_post_init__.

That being said, I have no idea whether my needs here at all represent the majority of use-cases.

@hynek
Copy link
Member Author

hynek commented Jan 20, 2019

I love myself some value types but frozen=True ain’t happening. :)

It just doesn’t fit Python too well. But IIRC we’ve decided on an own function for it?

@berislavlopac
Copy link

berislavlopac commented Jan 20, 2019

I agree with frozen=False as the default, but kinda like using the term mutable, unless there is a significant difference between the meaning in attrs and Python in general. So I guess I would vote for the mutable=True as default.

Oh, and I think that @bskinn's argument against frozen=True equally applies to slots=True; I still think they should both be off by default. Unless I'm mistaken, slots applies to the attributes defined via attrs and no additional attributes can't be added, is that right?

@berislavlopac
Copy link

No, but you could probably use metaclasses and our programmatic interface (make_class) to build it yourself?

Possibly, will look into it.

@hynek
Copy link
Member Author

hynek commented Jan 21, 2019

The reason why it's called frozen is because that's how we call these things in Python (frozenset etc) and I don't want to open a new nomenclature out of purity. I've seen good things fail too often because of the stubbornness of its maintainers to be a good Python citizen.

And yes, slots=True prevents new attributes and you really shouldn't add ad-hoc attributes so that's a feature. If you need it, you can always say slots=False and the class has a warning attached to itself, that the attribute list is not comprehensive.

@jml
Copy link

jml commented Jan 21, 2019

we’ve decided on an own function for it?

Did we? Where can I read about that?

@hynek
Copy link
Member Author

hynek commented Jan 21, 2019

Where can I read about that?

#408 (comment)

It's gonna be @attrs.define and @attrs.frozen. Depending on https://hynek.me/about/#gratitude I might add an attrs.mutable alias for define. ;)

@hynek
Copy link
Member Author

hynek commented Jan 21, 2019

Depending on https://hynek.me/about/#a-name-gratitude-a-gratitude I might add an attrs.mutable alias for define. ;)

It’s working 😱! Thank you so much @berislavlopac! ❤️

@berislavlopac
Copy link

Keep up the good work! 😉

@bskinn
Copy link

bskinn commented Jan 21, 2019

I actually think slots=True has a useful proofreading function, and thus has more (albeit definitely not iron-clad) merit as a default as compared to frozen: slots=True guards against typos in external code that uses the class.

I agree that a default slots=True has the potential to introduce confusing errors, but this at least might be mitigated by customizing the exception message... something like: "... Either you have made a typo in accessing a member, or you should define this class with @attr.s(slots=False)".

@berislavlopac
Copy link

Just a thought -- if we're using separate decorators for defining frozen and mutable classes, perhaps the former can have slots=True and the latter slots=False? 🤔 Just thinking out loud...

@bskinn
Copy link

bskinn commented Jan 21, 2019

Definitely makes sense to me for slots=True to be default on @attrs.frozen.

That could be one distinction between (a possibly yet-again-renamed) .define and .mutable... slots=True on the former and slots=False on the latter...

@gabbard
Copy link
Member

gabbard commented Jan 22, 2019

I also agree that frozen=True should not be the default for reasons @hynek gives above, but I wanted to note that @BSkin's use-case above works fine for frozen classes with a small tweak. Rather than:

@attr.s(slots=True)
class Foo:

    input_var = attr.ib()
    expensive_derived_var = attr.ib(init=False)

    def __attrs_post_init__(self):
        self.expensive_derived_var = self.calculate_expensive_thing()

    def calculate_expensive_thing(self):
        ...
        {expensive stuff with self.input_var}
        ...

do

@attr.s(slots=True, frozen=True)
class Foo:

    input_var = attr.ib()
    expensive_derived_var = attr.ib(init=False)

    @expensive_derived_thing.default
    def calculate_expensive_thing(self):
        ...
        {expensive stuff with self.input_var}
        ...
        return {computed value}

I wonder if this pattern is common enough to merit an example in the documentation.

Note that these derived attributes still participate in hash, cmp, etc. unless you turn them off (#310).

@berislavlopac
Copy link

That could be one distinction between (a possibly yet-again-renamed) .define and .mutable... slots=True on the former and slots=False on the latter...

On one hand, I like this approach, as it provides a number of different ways to skin this cat. On the other, I feel this goes against "one obvious way to do it", introducing simply too many combinations to keep in mind:

  • define with slots=True and frozen=False
  • mutable with slots=False and frozen=False
  • frozen with slots=True and frozen=True

So I'm not sure. 😆 Overall, I think I'm leaning slightly towards simplicity:

  • define with slots=False and frozen=False
  • frozen with slots=True and frozen=True

Especially if there are more differences between the two methods than just the default values of slots and frozen. And I can live without the mutable alias.

@wsanchez
Copy link

Yeah. I don't think mutable and define should be different. The intent with that alias wasn't to complicate the matrix but to allow pedantic folks like me to be explicit about whether a class is mutable or frozen, as opposed to "the usual sort" or frozen.

@hynek hynek pinned this issue Jan 23, 2019
@hynek
Copy link
Member Author

hynek commented Jan 23, 2019

Yeah there's not gonna be any more diversions. It's bad enough to have attrs.frozen and attrs.define. :)

@Tinche Tinche unpinned this issue Jan 23, 2019
@Tinche Tinche pinned this issue Jan 23, 2019
@petergaultney
Copy link

petergaultney commented Feb 1, 2019

Not to derail an excellent conversation about mutability, but has attrs ever considered building in support for disallowing 'empty' values for attributes that are not assigned a default?

For instance, in my own little world, I can't think of a single case where, given the type

@attr.s(auto_attribs=True)
class MyDatabaseType:
    username: str
    firstname: str
    email_addresses: List[str]

    favorite_colors: List[str] = attr.Factory(list)
    bio: str = ''

I would ever want to allow empty strings or lists for the attributes where I did not specifically provide a default. To me, making something optional is the same thing as providing a default, and vice-versa - under all other circumstances (well, at least for strings and collections types), an empty value is a sign of failure.

Obviously I can write validators for each of these items, but that ends up being at least 2 extra lines in the class definition for each attribute, or a single, very long line using attr.ib syntax, where previously the class definition was extremely clean.

I am probably totally wrong about all this, and would love to have it explained to me why. But if I'm not, may I suggest adding a flag to the decorator that, if provided, enables this basic level of validation for all str and list/set/other collection types for which a default value is not provided?

Again, I apologize for jumping into this ongoing conversation about better defaults - this is probably a simple feature request. But as it seems like default behavior to me, I couldn't resist. Thanks for an incredible project that has made programming with classes/typing in Python literally 500% more pleasant!

@wsanchez
Copy link

wsanchez commented Feb 4, 2019

@petergaultney That sounds a bit off-topic for this thread, which is more about cleaning up the API around existing functionality. You're asking for a new feature, which you've filed as #495; it'll be easier to track that conversation there.

@decentral1se
Copy link

Thanks for this effort, great stuff. I would humbly submit #391 for consideration. Stripping private attrs should not happen by default and instead be configurable with private_stripping=True or something like that.

@glyph
Copy link
Contributor

glyph commented May 13, 2020

We are about to standardize on from attr import dataclass everywhere in our codebase so I'm just wondering if there's been any movement on this lately :)

@hynek
Copy link
Member Author

hynek commented May 14, 2020

Well, I'm annoyed of typing @attr.s(auto_attribs=True, auto_exc=True, slots=True) as the next person.

And I've been cranking out PRs to fix all things that I want fixed before import attrs (notably #642, #635, and #607) but sadly nobody was reviewing so I just merged after two weeks of silence. So that takes a while with all the timeouts.

The last big remaining thing are pre-attribute on_setattr hooks that allow for both freezing single attributes and force validation on setting attributes. Been procrastinating on the spec for a while now but attr.ib(on_setattr=run_validator) seems like the correct default value and it would be a shame to not include it.

@berislavlopac
Copy link

Well, I'm annoyed of typing @attr.s(auto_attribs=True, auto_exc=True, slots=True) as the next person.

Perhaps a solution might be a "templating" approach, e.g. with some kind of a AttrsTemplate class or namedtuple:

my_template_1 = AttrsTemplate(auto_attrib=True)
my_template_2 = AttrsTemplate(auto_attrib=True, auto_exc=True)

@attr.s(template=my_template_1)
class Foo:
   ...

The last big remaining thing are pre-attribute on_setattr hooks that allow for both freezing single attributes and force validation on setting attributes

Oooh I just needed that recently, ended up using metadata to denote "mutability" of an attribute.

@hynek
Copy link
Member Author

hynek commented May 14, 2020

Work has started in #645!

@petergaultney
Copy link

@berislavlopac in theory the templating approach can be accomplished by meta-decorators, right?

def my_template_1(cls=None, **kwargs):
    def decorator(cls):
        return attrs(cls, **kwargs, auto_attrib=True, auto_exc=True)
    return decorator(cls) if cls else decorator

I wonder if either documenting this pattern or providing some sugar for it, e.g.

def attrs_template(**template_kwargs):
    def the_template(cls=None, **attrs_kwargs):
        def decorator(cls):
            merged_kwargs = {**attrs_kwargs, **template_kwargs}
            return attrs(cls, **merged_kwargs)
        return decorator(cls) if cls else decorator
    return the_template

my_template_1 = attrs_template(auto_exc=True, auto_attribs=True)

@my_template_1(frozen=True)
class Foo:
    bar: int

@berislavlopac
Copy link

Excellent point, haven't though of this approach -- it would definitely be useful to at least be documented if not implemented in this way.

@Tinche
Copy link
Member

Tinche commented May 14, 2020

@berislavlopac @petergaultney I use this approach all the time (https://github.com/Tinche/flattrs/blob/master/src/flattr/_fb_attrs.py#L35). Note that it needs special boilerplate if you're using mypy, and mypy doesn't perfectly support it.

@bitbucketboss
Copy link

bitbucketboss commented May 20, 2020

attr.ib(on_setattr=run_validator)

Perhaps attr.ib(on_setattr=validate) would be a little more concise?

@hynek
Copy link
Member Author

hynek commented May 21, 2020

attr.ib(on_setattr=run_validator)

Perhaps attr.ib(on_setattr=validate) would be a little more concise?

That’s not really an issue since it’s gonna be the default in upcoming APIs. I don’t think we need to pollute our APIs with any sugar for it in the meantime.

@oakkitten
Copy link

You can simply use partial:

>>> from functools import partial
>>>
>>> auto = partial(attrs, auto_attribs=True, auto_exc=True)
>>>
>>> @auto(frozen=True)  # additional parameters
... class Foo:
...     foo: int
...
>>> @auto               # no parameters and no parentheses
... class Bar:
...     bar: str
...
>>> Foo?
Init signature: Foo(foo: int) -> None
  ...
>>> Bar?
Init signature: Bar(bar: str) -> None
  ...

You can even stack these:

>>> frozen = partial(auto, frozen=True)

Given how easy and readable and pythonic this is, I think having the least magical defaults is very reasonable. Perhaps with the exception of cache_hash, are there reasons to not enable this by default for frozen instances?

Mypy is the bigger problem here, as it currently doesn't understand partial, and doesn't even understand auto = attrs; @auto; class ...

P.S. I personally would like to be able to drop parentheses on attrib like this:

@attrs
class Foo:
    foo = attrib

@hynek
Copy link
Member Author

hynek commented Jun 29, 2020

Sadly the mypy angle ruined this approach that I used to use all the time. :(

@petergaultney
Copy link

petergaultney commented Jun 29, 2020

Mypy is the bigger problem here, as it currently doesn't understand partial, and doesn't even understand auto = attrs; @auto; class ...

This partial application decorator-forwarding usecase can be managed with a micro-plugin for mypy - you basically need something that says:

import typing as ty
from mypy.plugin import Plugin, ClassDefContext
from mypy.plugins.attrs import attrs_class_maker_callback

MODULE_PATH_TO_YOUR_DECORATOR = "your.module.path.autoattrs"

class AutoAttrsDecoratorPlugin(Plugin):
    def get_class_decorator_hook(
        self, fullname: str
    ) -> ty.Optional[ty.Callable[[ClassDefContext], None]]:
        def fwd_auto(cls_def_ctx: ClassDefContext):
            if fullname == MODULE_PATH_TO_YOUR_DECORATOR:
                attr_class_maker_callback(cls_def_ctx, True)
        if fullname == MODULE_PATH_TO_YOUR_DECORATOR:
            return fwd_auto
        return None

def plugin(_version):
    return AutoAttrsDecoratorPlugin

What this doesn't support is things like frozen - however the mypy attrs plugin itself could be trivially rewritten to expose those parameters directly for these sorts of mypy plugins.

@hynek do you think mypy would accept a PR to make the built-in plugin more directly reusable? This whole mess of plugin code could probably be rewritten to be a one-liner that 'registers' a decorator with the mypy plugin.

@hynek
Copy link
Member Author

hynek commented Jun 29, 2020

@hynek do you think mypy would accept a PR to make the built-in plugin more directly reusable? This whole mess of plugin code could probably be rewritten to be a one-liner that 'registers' a decorator with the mypy plugin.

I've opened an issue asking for this almost two years ago: python/mypy#5406

FWIW, #650 is of interest here too.

@oakkitten
Copy link

doesn't even understand auto = attrs;

by the way, while assignment doesn't make mypy happy, importing under a different name does:

if TYPE_CHECKING:
    from attr import attrs as slotted
else:
    slotted = functools.partial(attrs, slots=True)

@bionicles
Copy link

bionicles commented Jul 19, 2020

one thing which breaks some functional programming stuff, is the fact you can't do bracketed lookup with attrs objects

a = {"name": "a"} 
a["name"] 
# a

@attrs
class Person:
    name: str = attrib()

a = Person(name="a")
a["name"]
# TypeError: 'Person' object is not subscriptable

means you can't do functional lenses, lookPath, assocPath, dissocPath, evolvePath, are all legit, makes me miss Ramda!
how can we make attrs better for functional programming?

@berislavlopac
Copy link

berislavlopac commented Jul 19, 2020

You can always implement your own __getitem__...

@bionicles
Copy link

    def __getitem__(self, key):
        return self.__getattribute__(key)

worked like a charm, thanks for reminding me, that would be a fun default to add!

@hynek
Copy link
Member Author

hynek commented Aug 14, 2020

Please check out #666 y'all.

@hynek
Copy link
Member Author

hynek commented May 5, 2021

So FTR, NG APIs will ship as stable in 21.1, now is the last chance to ask for changes in APIs that are annoying.

So far there's #705 but I'm sure there might be more?

To be clear: define/mutable/frozen/field are set in stone now. But there's a buttload of other APIs that may benefit from polish?

@hynek hynek pinned this issue May 6, 2021
@botant
Copy link
Contributor

botant commented May 6, 2021

I just want to say thanks for order=False by default. 🙏

@hynek hynek mentioned this issue Dec 15, 2021
10 tasks
@hynek hynek unpinned this issue Dec 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Thinking Needs more braining.
Projects
None yet
Development

Successfully merging a pull request may close this issue.