Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add Attribute.alias #950

Merged
merged 24 commits into from
Nov 30, 2022
Merged

Add Attribute.alias #950

merged 24 commits into from
Nov 30, 2022

Conversation

asford
Copy link
Contributor

@asford asford commented Apr 9, 2022

Summary

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.

Add an explicit & overridable per-attribute alias in attrs, mirroring the behavior described in the PEP and dataclass_transform specification. This allows 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.

This feature unblocks implementation of a shim layer to support attrs/dataclass interop, discussed in:

closes: #945
closes: #572

Goals:

  • This is silent no-op for "normal" users of attrs not using alias. Libraries building on attrs may use alias to detect both existing private-name init aliases and user-supplied aliases.
  • Add an alias : Optional[str] parameter to attr.ib et al and property to Attribute.
  • Lift current private-attribute-naming logic (name.lstrip("_")) into _ClassBuilder so that alias is always populated with the field's __init__ parameter name, regardless of whether an explicit alias is provided, during __init__ rendering and attr.fields introspection.
  • Leave Attribute.alias as None if not explicitly specified, so that field_transformer implementations can (a) create attributes without needing to be aware of alias and optionally provide alias overrides before attrs.
  • Update attr.evolve to use Attribute.alias rather than re-implemented name-mangling.
  • Update the attribute and private-attribute-handling documentation to note the new alias parameter.
  • Expand dataclass_transform documentation to describe how alias can be used to inform type checkers of private-name renaming behavior.
  • Politely file a bug against attrs noting the inconsistency between documentation and behavior. Dunder-methods are still affected by attr.ibs that will be __name-mangled should be attrs-init-mangled to <name> not ClassName_<name> #619, but evolve failures in pathological cases are resolved.
  • File a tracking issue against against cattrs to access Attribute.alias during codegen iff available in attrs: Use Attribute.alias for init argument names if attrs>= 2022.2 cattrs#322

Non-goals:

  • Modification of the existing private-attribute-naming logic, though could be tackled pre-release in a follow-up PR.

Pull Request Check List

  • Added tests for changed code.
    Our CI fails if coverage is not 100%.
  • New features have been added to our Hypothesis testing strategy.
  • Changes or additions to public APIs are reflected in our type stubs (files ending in .pyi).
    • ...and used in the stub test file tests/typing_example.py.
    • If they've been added to attr/__init__.pyi, they've also been re-imported in attrs/__init__.pyi.
  • Updated documentation for changed code.
    • New functions/classes have to be added to docs/api.rst by hand.
    • Changes to the signature of @attr.s() have to be added by hand too.
    • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
      Find the appropriate next version in our __init__.py file.
  • Documentation in .rst files is written using semantic newlines.
  • Changes (and possible deprecations) have news fragments in changelog.d.
  • Consider granting push permissions to the PR branch, so maintainers can fix minor issues themselves without pestering you.

docs/extending.rst Outdated Show resolved Hide resolved
Copy link
Member

@hynek hynek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks all very good and thorough, just a bunch of questions and nits.

it also needs a newsfragment. make it sounds excited. :)

sorry for taking so long…whenever Attribute changes the diffs make me wann hide under the bed.

src/attr/_make.py Outdated Show resolved Hide resolved
src/attr/_make.py Outdated Show resolved Hide resolved
docs/extending.rst Outdated Show resolved Hide resolved
src/attr/_funcs.py Show resolved Hide resolved
src/attr/_make.py Outdated Show resolved Hide resolved
docs/init.rst Show resolved Hide resolved
@hynek
Copy link
Member

hynek commented May 25, 2022

Anything you need from my end? 😇

@hynek hynek added this to the 22.2.0 milestone Aug 11, 2022
Copy link
Contributor Author

@asford asford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My turn to apologize for the slow turnaround.

I've applied the review comments, and believe this is ready for a final pass.

docs/init.rst Show resolved Hide resolved
src/attr/_make.py Outdated Show resolved Hide resolved
src/attr/_make.py Outdated Show resolved Hide resolved
src/attr/_make.py Outdated Show resolved Hide resolved
@asford asford requested a review from hynek November 13, 2022 17:28
@hynek
Copy link
Member

hynek commented Nov 16, 2022

w00t! Thanks for coming back! I'll try to be faster this time! :)

@asford
Copy link
Contributor Author

asford commented Nov 16, 2022

Really appreciate your work on attrs.
Thanks for being stoked on the contribution!

Copy link
Member

@hynek hynek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nailed it a tiny nit aside!

Thank you so much for coming back, I know how difficult it is to pick up something like this after a long time.

src/attr/_make.py Outdated Show resolved Hide resolved
@hynek hynek enabled auto-merge (squash) November 30, 2022 14:36
@hynek hynek merged commit ee3ecb1 into python-attrs:main Nov 30, 2022
@Tinche
Copy link
Member

Tinche commented Nov 30, 2022

Cool. Cattrs will be no problem, I can just bump the dependency to the latest version of attrs and implement it.

Mypy will be a bigger issue I think.

@hynek
Copy link
Member

hynek commented Nov 30, 2022

given alias is a dataclass transform feature, shouldn't it work OOB?

@Tinche
Copy link
Member

Tinche commented Nov 30, 2022

Unsure, Mypy has a custom attrs plugin so I don't know if the dataclass_transform code path is in charge of this.

@hynek
Copy link
Member

hynek commented Nov 30, 2022

aw looks like you're right. pyright works: #1063

mypy doesn't:

Adding

AliasExample(without_alias=42, _with_alias=23)

gives me: tests/typing_example.py:134: error: Unexpected keyword argument "_with_alias" for "AliasExample"; did you mean "with_alias" or "without_alias"?

@Tinche 🐶🥺?

@Tinche
Copy link
Member

Tinche commented Nov 30, 2022

Yeah, yeah. But we'd need to release attrs first.

@hynek
Copy link
Member

hynek commented Nov 30, 2022

yeah, it's super overdue too.

i'll try to get #1003 in ASAP (it's just an alias, so it should be just busywork) and push it out RSN.

querti added a commit to querti/pushsource that referenced this pull request Jan 5, 2023
Since the attrs version release, pub tests have begun failing with the
error "TypeError: keywords must be strings". Attrs have added an "alias"
parameter to each attribute[1], which is used in the attrs's "evolve"
method to construct a new instance. This causes issues with the attribute
"from", which has some special logic implemented in order to be usable
at all[2]. The attribute "from" is generated from "from_", copying all
its parameters. Since "alias" is a new parameter, it isn't copied and
thus its value is set no "None". attrs's "evolve" method uses **kwargs
to create a new instance, and it sets one of the keys in **kwargs to
"None" (from alias). Keys in **kwargs must be strings, which causes the
TypeError. It can be fixed by also setting "alias" in the newly created
"from" attribute. In case of "from_", it has the same value as "name"
parameter ("from_"), which is why we don't want to copy it from the old
attribute, but explicitly set it to "from" (otherwise it would be
"from_"). The change should be backwards compatible with older versions
of attrs.

[1] python-attrs/attrs#950
[2] release-engineering#108
querti added a commit to querti/pushsource that referenced this pull request Jan 5, 2023
Since new attrs version release, pub tests have begun failing with the
error "TypeError: keywords must be strings". Attrs have added an "alias"
parameter to each attribute[1], which is used in the attrs's "evolve"
method to construct a new instance. This causes issues with the attribute
"from", which has some special logic implemented in order to be usable
at all[2]. The attribute "from" is generated from "from_", copying all
its parameters. Since "alias" is a new parameter, it isn't copied and
thus its value is set no "None". attrs's "evolve" method uses **kwargs
to create a new instance, and it sets one of the keys in **kwargs to
"None" (from alias). Keys in **kwargs must be strings, which causes the
TypeError. It can be fixed by also setting "alias" in the newly created
"from" attribute. In case of "from_", it has the same value as "name"
parameter ("from_"), which is why we don't want to copy it from the old
attribute, but explicitly set it to "from" (otherwise it would be
"from_"). The change should be backwards compatible with older versions
of attrs.

[1] python-attrs/attrs#950
[2] release-engineering#108
querti added a commit to querti/pushsource that referenced this pull request Jan 5, 2023
Since new attrs version release, pub tests have begun failing with the
error "TypeError: keywords must be strings". Attrs have added an "alias"
parameter to each attribute[1], which is used in the attrs's "evolve"
method to construct a new instance. This causes issues with the attribute
"from", which has some special logic implemented in order to be usable
at all[2]. The attribute "from" is generated from "from_", copying all
its parameters. Since "alias" is a new parameter, it isn't copied and
thus its value is set no "None". attrs's "evolve" method uses **kwargs
to create a new instance, and it sets one of the keys in **kwargs to
"None" (from alias). Keys in **kwargs must be strings, which causes the
TypeError. It can be fixed by also setting "alias" in the newly created
"from" attribute. In case of "from_", it has the same value as "name"
parameter ("from_"), which is why we don't want to copy it from the old
attribute, but explicitly set it to "from" (otherwise it would be
"from_"). The change should be backwards compatible with older versions
of attrs.

[1] python-attrs/attrs#950
[2] release-engineering#108
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
3 participants