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

Add PEP681 explicit & overridable per-attribute __init__ alias. #945

Closed
asford opened this issue Apr 2, 2022 · 15 comments · Fixed by #950
Closed

Add PEP681 explicit & overridable per-attribute __init__ alias. #945

asford opened this issue Apr 2, 2022 · 15 comments · Fixed by #950

Comments

@asford
Copy link
Contributor

asford commented Apr 2, 2022

PEP681 introduces an optional alias parameter to field descriptors, which specifies an alternative __init__ name for the member. This is not implemented in dataclasses, but is implemented in pydantic.

It would be useful to implement an explicit & overridable per-attribute alias in attrs, mirroring the behavior described in the PEP and dataclass_transform specification. This would allow users to directly address the single largest incompatibility noted in the specification, attrs private attributes handling, which strips leading underscores from member names, discussed in #795.

Additionally, though we all love attrs as a tastemaker with strongly held principles, "private" attribute handling appears to have been (be?) a significant source of confusion and is likely causing at least one active attrs-internal bug. This has also been a source of issues in downstream libraries, noted in references below.

In the longer-term, this would enable additional optional configuration flags to bring attrs into a strict-superset of the behavior of dataclasses, as roughly noted in #565 and #686. Enabling this work is a design goal here, but outside the scope of this issue.

A (simple?) solution may be to extend attr.ib with an optional alias parameter, which sets the __init__ name for the field.
This would, as is the custom, be added to attrs.Attribute and would be available during for introspection in attr.fields and customization in the field_transformer hook.
If alias is unspecified it will be set to a str, indicating the attribute's argument name in the generated __init__.

For example in the current behavior, alias = name.lstrip("_") so that:

>>> @attr.define
... class Foo:
...    bar: str
...    _bat: str
>>> attr.fields_dict(Foo)
... { "bar": Attribute(name="bar", alias="bar", ...), "_bat": Attribute(name="_bat", alias="bat", ...) }

We would retain the default private attribute renaming behavior in this implementation, which would allow users to either adjust the alias of the attribute to match their preferences or inform dataclass_transform consumers of the attribute's __init__ name. Ie:

@attr.define
class InformDataclassTransform:
    bar: str
    _bat: str = attr.ib(alias="bat")

@attr.define
class AliasMyWay:
   bar: str
   _bat: str = attr.ib(alias="_bat")

Refs:

Related issues:
#391 - Pending feature request, would be closed.
#244 - Class of issue introduced by behavior, would have workaround.
#619 - Feature request with no clear single answer, would have workaround.
#171 - Some original confusion about this behavior.

This is currently implemented in _make via arg_name = a.name.lstrip("_").
This is modeled in evolve via init_name = attr_name if attr_name[0] != "_" else attr_name[1:].

The lstrip/slice inconsistency here is almost certainly a bug for non-idiomatic cases like:

@attr.define
class NonIdiomatic:
    ___triple_underscore: str

Exposing alias may be useful for libraries which interact with attrs-generated __init__ methods.
Typified by cattrs, which re-implement's private-attribute naming via ian = an if (is_dc or an[0] != "_") else an[1:].
Note, this implementation is also affected by lstrip/slice inconsistency.
This could be replaced by a lookup of the target Attribute alias.

Originally posted by @asford in #686 (comment)

@hynek
Copy link
Member

hynek commented Apr 7, 2022

This sounds like a great excuse to fix a very old desire!

I would like to bike-shed the name on attribute for a moment tho. I find alias a bit ambiguous and potentially misleading, init_name would explain better, what it's for, no? Or are there other places that this would play a role?

The PEP says:

alias is an optional str parameter that provides an alternative name for the field. This alternative name is used in the synthesized __init__ method.


I regret to inform you that the current algorithm is not lstrip("_") – it only removes one underscore. 😬

@asford
Copy link
Contributor Author

asford commented Apr 7, 2022

I agree in that I don't like the color of aIias either, but my goal on this proposal is to land on behavior that's consistent with the PEP681 proposal. This is motivated by wanting the dataclass transform type checking to work "out of the box".

Unfortunately, there's already a splash of paint here that's been set by pydantic on the dataclass_transform spec and, by extension, PEP681. I believe we (but let's not kid ourselves, you're the one with actual influence here 😉) could jump in before the paint on the PEP fully dries, but I think this may require hasty action!

I'd mildly prefer to avoid a name split akin factory/default_factory, and just go with alias but would happy defer if you've strong feelings.

@asford
Copy link
Contributor Author

asford commented Apr 7, 2022

Regretfully, we are both incorrect here.
I'd overstated the impact of the bug, it only triggers when there are multiple underscores in the prefix but none in the suffix. 😨

There's some semi-spooky interaction here due to name mangling in the declaration that I haven't fully tracked down.

[ins] In [10]: @attr.define
          ...: class FooPathological:
          ...:     _priviate: int
          ...:     __mangled: int
          ...:     ___manytrailing_error__: int
          ...:

[ins] In [11]: FooPathological.__init__?
Signature:
FooPathological.__init__(
    self,
    priviate: int,
    FooPathological__mangled: int,
    manytrailing_error__: int,
) -> None
Docstring: Method generated by attrs for class FooPathological.
File:      ~/ab/main/<attrs generated init __main__.FooPathological-4>
Type:      function

@sscherfke
Copy link
Contributor

Would solving this issue make life easier for projects that alias or wrap attrs.define() and attrs.field()?

@hynek
Copy link
Member

hynek commented Apr 7, 2022

To be clear, I'm not asking the argument to be renamed, just the attribute on the Attribute class but I guess it's not worth to add confusion by being more correct…

As for the underscores: I just remember it did something wrong with dunders. 😅

JFTR: they've added factory 🎉

Would solving this issue make life easier for projects that alias or wrap attrs.define() and attrs.field()?

I'm not sure what this means TBH.

@wbolster
Copy link
Member

wbolster commented Apr 7, 2022

niiiiiiice seems i arrive just in time for some naming bikeshedding 🙃

the attr.field(init=...) argument is currently a bool, but it could become bool | str so that no new argument (which needs a name) is needed.

weird, you say? nah! there is prior art: the repr= argument behaves similarly! grep for _ReprArgType or look at #568

@hynek
Copy link
Member

hynek commented Apr 8, 2022

while I love a good bikeshed, but the whole point is that alias is coming from PEP 681 which means we'd get pylance for free. :) I wouldn't want to go our own way

@hynek
Copy link
Member

hynek commented Apr 9, 2022

@asford do you need anything to proceed?

@sscherfke
Copy link
Contributor

Would solving this issue make life easier for projects that alias or wrap attrs.define() and attrs.field()?

I'm not sure what this means TBH.

Currently, you need a mypy-plugin if you alias or wrap attrs.define() (or attrs.field()). And if a lib does that, all users of that lib needed to do that if they used the attrs classes provided by that lib.

The question is if this issue would help mypy figuring out that a decorator is an alias or wrapper for attrs.define() or attrs.field().

@hynek
Copy link
Member

hynek commented Apr 9, 2022

This is about aliasing field names in __init__, not APIs.

Quoth the PEP:

alias is an optional str parameter that provides an alternative name for the field. This alternative name is used in the synthesized __init__ method.

e.g.

@define
class C:
     x: int = field(alias="y")

C(y=42)

@asford asford changed the title Add explicit & overridable per-attribute __init__ alias. Add PEP681 explicit & overridable per-attribute __init__ alias. Apr 9, 2022
@asford
Copy link
Contributor Author

asford commented Apr 9, 2022

Would solving this issue make life easier for projects that alias or wrap attrs.define() and attrs.field()?

I'm not sure what this means TBH.

Currently, you need a mypy-plugin if you alias or wrap attrs.define() (or attrs.field()). And if a lib does that, all users of that lib needed to do that if they used the attrs classes provided by that lib.

The question is if this issue would help mypy figuring out that a decorator is an alias or wrapper for attrs.define() or attrs.field().

If mypy implements the dataclass_transform spec, which I would anticipate once PEP681 is "accepted", then the library-authors will be able to decorate their attrs-wrappers with dataclass_transform and have them treated as a wrapper. This is the current behavior of pyright.

This specific feature expands attrs's, and potentially by extension any attrs-wrapping library's, support for the spec.

As the spec does not cover all of attrs's splendid features, one could imagine a mypy plugin tailored for attrs patterned on dataclass_transform that supports additional attrs-specific features. Ie, an attrs_transform decorator. Such a decorator is wildly outside the scope of this issue. 😉

@asford
Copy link
Contributor Author

asford commented Apr 9, 2022

@asford do you need anything to proceed?

edit: Move goals to spike in #950

@asford asford mentioned this issue Apr 9, 2022
21 tasks
@Tinche
Copy link
Member

Tinche commented Apr 9, 2022

Let's do this.

My $0.02: the name is a little unfortunate, it's just the __init__ alias right? It should have been named init_alias then. The actual attribute is still the original name. Do we use the alias in __repr__ too? (Guessing not.)

As for cattrs, the technical part will be easy, but what about the UX? If you unstructure C, is the alias or the attribute name used for the dict key? Right now it's the attribute name. I can see a case for both, for example if you're dealing with an API that uses a Python keyword for a field name, like from.

@asford
Copy link
Contributor Author

asford commented Apr 11, 2022

It should have been named init_alias then

Yes, I agree with you both that the same is unfortunate. I'm happy to implement @hynek's suggestion of having a different name on Attribute like init_alias, but as noted this introduces it's own form of confusing mismatch between the attr.ib parameter and the Attribute field. My soft inclination is to grit teeth 😬, use alias, and try to document it well.

As for cattrs, the technical part will be easy, but what about the UX? If you unstructure C, is the alias or the attribute name used for the dict key? Right now it's the attribute name. I can see a case for both, for example if you're dealing with an API that uses a Python keyword for a field name, like from.

My goal for this feature is that it's a silent no-op for any existing users, but that alias can be used to detect attrs's existing private-name init aliasing logic.

I believe we should leave the semantics of cattrs unchanged: the dict key is the attribute name, not the __init__ name of the property. It would be very hard to fulfill the goal above without breaking cattrs-users (like myself 😉) who depend on cattrs's current "attribute name is dict key" property.Users can (and like myself, do) use the cattrs rename customization hook to adjust the unstructured dict-key for attributes when required.

Keeping these two behaviors independent and composable, with alias just controlling init behavior and a separate cattrs-based rename controlling the unstructured representation eels like the correct course here.

@nrbnlulu
Copy link

possible fix for #417

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants