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

continuing #839: Extend attrs brain to support provisional APIs #1187

Merged
merged 12 commits into from
Sep 28, 2021
Merged

continuing #839: Extend attrs brain to support provisional APIs #1187

merged 12 commits into from
Sep 28, 2021

Conversation

jstriebel
Copy link
Contributor

Description

This PR continues the original PR #839, which seems to be stale. Only thing missing are formatting and obeying the linter.
From the original PR:

With attrs 20, new "provisional" APIs were added (see https://www.attrs.org/en/stable/api.html?highlight=field#provisional-apis).

This PR adds support to those new functions in the existing attrs brain.

Type of Changes

Type
✨ New feature

Related Issue

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

The coverage is expected to fail don't worry about it coverall is down right now.

@Pierre-Sassoulas Pierre-Sassoulas added the Brain 🧠 Needs a brain tip label Sep 21, 2021
@jstriebel
Copy link
Contributor Author

@Pierre-Sassoulas Thanks for the fast review! I adapted the formatting, CI is green now, but needs an additional OK from a maintainer.

ChangeLog Outdated Show resolved Hide resolved
@jstriebel
Copy link
Contributor Author

@Pierre-Sassoulas Is there anything blocking here? Or could you merge this PR? Thank you!

@Pierre-Sassoulas
Copy link
Member

Hey @hippo91 could you review this, please ? I'm not confident enough to merge astroid brains change :)

@hippo91 hippo91 self-assigned this Sep 25, 2021
Copy link
Contributor

@hippo91 hippo91 left a comment

Choose a reason for hiding this comment

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

@jstriebel thanks for this PR. Everything looks ok. As i understand the new attrs API, define, mutable and frozen are in fact attr.s with specialization of the default arguments.
attrs.field is an alias of attrs.ib. So the modifications proposed here are ok for me even if i wonder about the utility of field and attrib names without attrs prefix.

astroid/brain/brain_attrs.py Show resolved Hide resolved
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.8.1, 2.8.2 Sep 25, 2021
@jstriebel
Copy link
Contributor Author

As i understand the new attrs API, define, mutable and frozen are in fact attr.s with specialization of the default arguments.

Exactly, this only adds some new decorator- and function-names that were added with the next-generation API (indeed with different defaults than before), the main mechanisms stay exactly the same.

@jstriebel
Copy link
Contributor Author

@Pierre-Sassoulas @hippo91 Is there anything missing to merge this? Or do you track this via the milestones and merge it later? Sorry to be bothering 🙏

@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.8.2, 2.8.1 Sep 28, 2021
@Pierre-Sassoulas
Copy link
Member

Thank you for the remainder, let's merge that in 2.8.1 as it's finished earlier than I thought :)

@Pierre-Sassoulas Pierre-Sassoulas merged commit fe918ff into pylint-dev:main Sep 28, 2021
@jstriebel jstriebel deleted the patch-1 branch September 28, 2021 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Brain 🧠 Needs a brain tip
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants