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

Fix regression property no member 2641 #890

Conversation

hippo91
Copy link
Contributor

@hippo91 hippo91 commented Feb 7, 2021

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

This PR adds the attr_fset method to the PropertModel class, so that pylint is able to deal with property setter.
The need for such a fix has been highlighted with #740 and is confirmed by the bug report pylint-dev/pylint#3480.

Type of Changes

Type
🐛 Bug fix

Related Issue

Closes pylint-dev/pylint#3480

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.

Is this ready for merging ? If it fixes pylint's master I4m eager to have it merged 😄

astroid/interpreter/objectmodel.py Show resolved Hide resolved
@hippo91
Copy link
Contributor Author

hippo91 commented Feb 10, 2021

@Pierre-Sassoulas yes it is. I was just waiting for a review so now it is ok.
I got a day off tomorrow so i will try to merge it. I was also thinking about releasing a new astroid version (2.5?).
Are you ok with it?

@Pierre-Sassoulas Pierre-Sassoulas merged commit 0d8ed3f into pylint-dev:master Feb 10, 2021
@Pierre-Sassoulas
Copy link
Member

Yeap, releasing astroid 2.5.0 seems like a good idea. Might be the time to use the new workflow with master becoming 2.6 and a creating a 2 .5 branch for bug fixes. I merged so I can fix pylint's master which I think will not work immediately :)

@Pierre-Sassoulas
Copy link
Member

There are 5 pylint's unit tests not passing with latest astroid 2.5, that seems to work with astroid 2.4.1, I don't know how to fix, I suppose that was why you said "trying to merge" :) Sorry if I merged too prematurely.

@hippo91
Copy link
Contributor Author

hippo91 commented Feb 10, 2021

It is weird. I did not see any failing tests on my laptop. Where can i see those failing tests? Anyway i will look at this problem tomorrow.

@cdce8p
Copy link
Member

cdce8p commented Feb 10, 2021

I'm seeing them only with 3.9, 3.8 works fine. It's always an unused-import error, so probably not related to this MR.

FAILED tests/test_functional.py::test_functional[unsubscriptable_value] - AssertionError: Wrong results for file "unsubscriptable_value":
FAILED tests/test_functional.py::test_functional[unpacking_non_sequence] - AssertionError: Wrong results for file "unpacking_non_sequence":
FAILED tests/test_functional.py::test_functional[unsupported_delete_operation] - AssertionError: Wrong results for file "unsupported_delete_...
FAILED tests/test_functional.py::test_functional[unsupported_assignment_operation] - AssertionError: Wrong results for file "unsupported_ass...
FAILED tests/test_functional.py::test_functional[invalid_exceptions_caught] - AssertionError: Wrong results for file "invalid_exceptions_cau...

Run with

pytest tests/test_functional.py

--

Edit: Noticed that I had an old astroid version installed in my 3.8 environment. Error is version independent.
I used the following to install the current astroid commit:

pip uninstall astroid
pip install -U git+git://github.com/PyCQA/astroid.git@0d8ed3f88deef0b2e85b5310b82cc3998220dc3c

@cdce8p
Copy link
Member

cdce8p commented Feb 10, 2021

After some more investigation the issue first appeared after #841 was merged.
Looks like the pylint tests just need to be updated, though I know to little about it to be sure.

@Pierre-Sassoulas
Copy link
Member

This MR's pipelines were launched after #890 got merged: (@cdce8p is right but there is also full traceback)

@hippo91
Copy link
Contributor Author

hippo91 commented Feb 11, 2021

@Pierre-Sassoulas @cdce8p thanks for your investigations.
I can confirm that #841 is the PR that triggers those failing tests. Those tests are the ones that are using six.with_metaclass (cf #841). So maybe there is a problem with this PR.
However, there is the pylint-dev/pylint#4006 that is here to remove six dependency. If we remove the six dependency on the failing unit tests modules then those ones are not failing anymore. I do not yet understand what is the criteria that @dgilman used to drop or keep six import in the unittests.
To conclude we are facing two possibilities: fixing a probable pb with #841 or totally drop the import of six module in pylint...

@hippo91 hippo91 mentioned this pull request Feb 11, 2021
2 tasks
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 this pull request may close these issues.

False positive E1101 (no-member) on overridden class property setters
3 participants