Bug pylint 4960 #1176

hippo91 · 2021-09-16T17:03:44Z

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

The PR #1148 introduced a regression that causes pylint-dev/pylint#4960.
While the idea behind #1148 may be a good idea, its implementation was faulty and prevent the use of brains even if the node that are modified inside those brains are not part of a potentially dynamically loaded module.
I didn't find a correct implementation that prevent the modification of nodes that are defined inside a dynamically loaded module.
That's why this PR is a partial revert of #1148. (revert concerns only the builder module).
I also added a numpy brain that allows pylint to infer correctly the call to numpy.ma.masked_where function so that pylint-dev/pylint#3342 will be definitely closed.

Type of Changes

	Type
✓	🐛 Bug fix

Related Issue

Closes pylint-dev/pylint#3342
Closes pylint-dev/pylint#4960

Pierre-Sassoulas · 2021-09-17T13:08:50Z

Hey, the issue with the CI was fixed, you can rebase on main :)

hippo91 · 2021-09-17T15:22:37Z

Thanks @Pierre-Sassoulas !

hippo91 · 2021-09-18T09:13:42Z

@Pierre-Sassoulas have you got an idea why the CI test is failing? Here is the stack:

 Submitting coverage to coveralls.io...
Could not submit coverage: 405 Client Error: Not Allowed for url: https://coveralls.io/api/v1/jobs
Traceback (most recent call last):
  File "/home/runner/work/astroid/astroid/venv/lib/python3.8/site-packages/coveralls/api.py", line 290, in submit_report
    response.raise_for_status()
  File "/home/runner/work/astroid/astroid/venv/lib/python3.8/site-packages/requests/models.py", line 953, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 405 Client Error: Not Allowed for url: https://coveralls.io/api/v1/jobs

DanielNoord · 2021-09-18T09:14:27Z

Coveralls is down. They are doing a database update it seems.

hippo91 · 2021-09-18T09:34:34Z

Thanks @DanielNoord !

Pierre-Sassoulas

Would it be possible to rebase on main instead of merging, please ? There's change from the main branch mixed in the diff so it's hard to review (especially considering that there was a lot of changes between f-strings migration and membership check migration from tuple to set)

hippo91 · 2021-09-19T17:08:48Z

@Pierre-Sassoulas i just rebase. Now there are only 4 files that are different!

ChangeLog

cdce8p

The change looks good. Just some small comments

ChangeLog

tests/unittest_brain_numpy_ma.py

cdce8p

One last comment.

--
@hippo91 It would be great if you don't resolve review comments yourself. That makes it really difficult to see any comments you might add. Better to just add 👍🏻 and let the person who left the comment resolve it.
(Accepting suggestions is somewhat different as Github auto-resolves those. No need to unresolve them afterwards.)

cdce8p · 2021-09-20T22:28:05Z

tests/unittest_brain_numpy_ma.py

+from astroid import builder
+
+
+@pytest.mark.skipif(not HAS_NUMPY, reason="This test requires the numpy library.")


Suggested change

@pytest.mark.skipif(not HAS_NUMPY, reason="This test requires the numpy library.")

@pytest.mark.skipif(HAS_NUMPY is False, reason="This test requires the numpy library.")

Technically the same. Might be just personal preference as this is how I think of it.

cdce8p · 2021-09-25T10:18:12Z

@hippo91 Could you rebase this PR onto main? It seems like there are again some unrelated commits in the history.

to prevent nodes that are dynamically imported to be inferred through an astroid's brain, the way it was done in builder.py was incorrect. The way it was done, lead to prevent the use of astroid legetimate brains even for node that was not dynamically loaded.

for more information, see https://pre-commit.ci

hippo91 · 2021-09-25T10:54:36Z

@cdce8p it should be ok now. Sorry for that. I don't know what kind of mess i did with this branch.

Pierre-Sassoulas added this to the 2.8.1 milestone Sep 16, 2021

hippo91 requested review from Pierre-Sassoulas and cdce8p and removed request for Pierre-Sassoulas September 19, 2021 10:13

Pierre-Sassoulas reviewed Sep 19, 2021

View reviewed changes

hippo91 force-pushed the bug_pylint_4960 branch from e34614e to dc8d45e Compare September 19, 2021 17:07

Pierre-Sassoulas approved these changes Sep 19, 2021

View reviewed changes

ChangeLog Outdated Show resolved Hide resolved

Pierre-Sassoulas added Brain 🧠 Needs a brain tip Bug 🪳 labels Sep 19, 2021

cdce8p added the pylint-tested PRs that don't cause major regressions with pylint label Sep 19, 2021

cdce8p reviewed Sep 19, 2021

View reviewed changes

ChangeLog Outdated Show resolved Hide resolved

tests/unittest_brain_numpy_ma.py Outdated Show resolved Hide resolved

tests/unittest_brain_numpy_ma.py Outdated Show resolved Hide resolved

tests/unittest_brain_numpy_ma.py Outdated Show resolved Hide resolved

cdce8p reviewed Sep 19, 2021

View reviewed changes

tests/unittest_brain_numpy_ma.py Outdated Show resolved Hide resolved

tests/unittest_brain_numpy_ma.py Outdated Show resolved Hide resolved

cdce8p approved these changes Sep 20, 2021

View reviewed changes

hippo91 and others added 9 commits September 25, 2021 12:45

Adds a brain to infer the numpy.ma.masked_where function

e0cf9bc

Adds two entries

eceb720

Reformat

75d29c8

Update ChangeLog

e28f629

Takes into account cdce8p remarks

79509e0

[pre-commit.ci] auto fixes from pre-commit.com hooks

ae51f3d

for more information, see https://pre-commit.ci

Takes into account cdce8p remarks

3600803

[pre-commit.ci] auto fixes from pre-commit.com hooks

14f23eb

for more information, see https://pre-commit.ci

hippo91 added 2 commits September 25, 2021 12:50

Takes into account last cdce8p remark

e659e11

Removes ueseless blank lines

a6a9c56

hippo91 force-pushed the bug_pylint_4960 branch from 29daa03 to a6a9c56 Compare September 25, 2021 10:52

[pre-commit.ci] auto fixes from pre-commit.com hooks

72248c8

for more information, see https://pre-commit.ci

Pierre-Sassoulas merged commit 05445e2 into pylint-dev:main Sep 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug pylint 4960 #1176

Bug pylint 4960 #1176

hippo91 commented Sep 16, 2021

Pierre-Sassoulas commented Sep 17, 2021

hippo91 commented Sep 17, 2021

hippo91 commented Sep 18, 2021

DanielNoord commented Sep 18, 2021

hippo91 commented Sep 18, 2021

Pierre-Sassoulas left a comment

hippo91 commented Sep 19, 2021

cdce8p left a comment

cdce8p left a comment •

edited

Loading

cdce8p Sep 20, 2021

cdce8p commented Sep 25, 2021

hippo91 commented Sep 25, 2021

		from astroid import builder


		@pytest.mark.skipif(not HAS_NUMPY, reason="This test requires the numpy library.")

	@pytest.mark.skipif(not HAS_NUMPY, reason="This test requires the numpy library.")
	@pytest.mark.skipif(HAS_NUMPY is False, reason="This test requires the numpy library.")

Bug pylint 4960 #1176

Bug pylint 4960 #1176

Conversation

hippo91 commented Sep 16, 2021

Steps

Description

Type of Changes

Related Issue

Pierre-Sassoulas commented Sep 17, 2021

hippo91 commented Sep 17, 2021

hippo91 commented Sep 18, 2021

DanielNoord commented Sep 18, 2021

hippo91 commented Sep 18, 2021

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

hippo91 commented Sep 19, 2021

cdce8p left a comment

Choose a reason for hiding this comment

cdce8p left a comment • edited Loading

Choose a reason for hiding this comment

cdce8p Sep 20, 2021

Choose a reason for hiding this comment

cdce8p commented Sep 25, 2021

hippo91 commented Sep 25, 2021

cdce8p left a comment •

edited

Loading