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

Jump to bottom

attrs namedtuple #11794

Merged
merged 4 commits into from Jan 7, 2022
Merged

attrs namedtuple #11794

merged 4 commits into from Jan 7, 2022
+86 −10

Conversation

@Tinche
Copy link
Contributor

@Tinche Tinche commented Dec 19, 2021 •
edited

This PR further enhances the attrs plugin by generating a more correct __attrs_attrs__ field.

The current version of the plugin generates this field as a tuple of attributes. This enhancement makes it a namedtuple, which is what attrs actually does in runtime. The difference is a large increase in the ergonomics of the field: instead of A.__attrs_attrs__[0], a user can access an attribute using A.__attrs_attrs__.x. Also note the end-users don't actually use the __attrs_attrs__ field directly, but access it using attr.fields, which is a very simple getter interface.

This might sound like a trivial change, but it has big repercussions down the line for static checks in libraries building on top of attrs.

I have added 2 extra tests and improved an existing one.
@Tinche Tinche changed the title Tin/attrs namedtuple attrs namedtuple Dec 19, 2021
sobolevn
sobolevn reviewed Dec 19, 2021
View changes
mypy/plugins/attrs.py Outdated Show resolved Hide resolved
@Tinche
Copy link
Contributor Author

@Tinche Tinche commented Dec 27, 2021

@sobolevn Hello, is there anything more I can do to nudge this along? (I tried the re-request review button, it did nothing.)

@sobolevn
Copy link
Contributor

@sobolevn sobolevn commented Dec 27, 2021

@Tinche Sorry, I am not a real mypy developer, I am just helping with what I can 🙂
You need to ping someone with proper review permissions.

@Tinche
Copy link
Contributor Author

@Tinche Tinche commented Dec 28, 2021

@euresti Hello, might I kindly ask for your feedback here?

@euresti
Copy link
Contributor

@euresti euresti commented Dec 29, 2021

@ilevkivskyi, can you give some advice on how to nudge this along?

ilevkivskyi
ilevkivskyi approved these changes Jan 6, 2022
View changes
Copy link
Collaborator

@ilevkivskyi ilevkivskyi left a comment

TBH I didn't work on this for a while, but I don't see anything obviously wrong. I would however recommend adding a fine-grained daemon test, where you define a class, then update type of an attribute, and check that the change is propagated correctly to different file in fine-grained mode.

I will leave this for someone else to merge when the fine-grained test is added.

@Tinche
Copy link
Contributor Author

@Tinche Tinche commented Jan 7, 2022

@ilevkivskyi thanks, added a fine-grained test. Hope I got it right!

@JelleZijlstra JelleZijlstra merged commit 254d41e into python:master Jan 7, 2022
14 checks passed
@Tinche Tinche deleted the tin/attrs-namedtuple branch Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sobolevn sobolevn

@JelleZijlstra JelleZijlstra

@ilevkivskyi ilevkivskyi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet
5 participants
@Tinche @sobolevn @euresti @JelleZijlstra @ilevkivskyi