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 __new__ and __call__ toObjectModel and ClassModel #1606

Merged
merged 5 commits into from
Jul 3, 2022

Conversation

DanielNoord
Copy link
Collaborator

Steps

  • For new features or bug fixes, add a ChangeLog entry describing what your PR does.
  • Write a good description on what the PR does.

Description

/CC @jacobtylerwalls

Part of #1519.

This was what I was talking about in #1519 (comment). __new__ and __call__ are part of the models for object and classes. Thus, instead of adding them as attr___ne__ I think it makes sense to try and define them.

One thing I wonder is whether this should indeed be properties. But looking at other methods I think they should? See for example:
https://github.com/PyCQA/astroid/blob/01db68b1b37fdfa4d7899fa1ba71015b33efb34b/astroid/interpreter/objectmodel.py#L288-L289

Type of Changes

Type
✨ New feature
🔨 Refactoring

@DanielNoord DanielNoord added Enhancement ✨ Improvement to a component pylint-tested PRs that don't cause major regressions with pylint labels Jun 8, 2022
@coveralls
Copy link

coveralls commented Jun 8, 2022

Pull Request Test Coverage Report for Build 2605029910

  • 21 of 21 (100.0%) changed or added relevant lines in 3 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.008%) to 92.118%

Files with Coverage Reduction New Missed Lines %
astroid/bases.py 1 87.35%
astroid/interpreter/objectmodel.py 2 92.6%
Totals Coverage Status
Change from base Build 2603040821: 0.008%
Covered Lines: 9431
Relevant Lines: 10238

💛 - Coveralls

@jacobtylerwalls
Copy link
Member

Thanks for pushing forward on this. I haven't found time to do a review yet, but I did quickly check it against pylint-dev/pylint#6402 and this doesn't pass its test case. Was that expected?

@DanielNoord
Copy link
Collaborator Author

Thanks for pushing forward on this. I haven't found time to do a review yet, but I did quickly check it against PyCQA/pylint#6402 and this doesn't pass its test case. Was that expected?

Yeah! Ultimately we should merge #1519 as well.

However, since __new__ and __init__ are used so ubiquitously I thought there would be benefits to defining them rather than making them attr___ne__. I think for the other dunders most tests will automatically pass if we define them on ObjectModel instead of FunctionModel. (Although I haven't checked).

@DanielNoord DanielNoord marked this pull request as ready for review June 24, 2022 13:10
astroid/bases.py Outdated Show resolved Hide resolved
astroid/bases.py Outdated Show resolved Hide resolved
astroid/interpreter/objectmodel.py Show resolved Hide resolved
Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Looks really good! One last Q

astroid/nodes/scoped_nodes/scoped_nodes.py Show resolved Hide resolved
@DanielNoord DanielNoord merged commit a6fdf0b into pylint-dev:main Jul 3, 2022
@DanielNoord DanielNoord deleted the new branch July 3, 2022 16:16
@DanielNoord DanielNoord added this to the 2.12.0 milestone Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component pylint-tested PRs that don't cause major regressions with pylint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants