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

Make FunctionDef.implicit_parameters return 1 for methods #1531

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ Release date: TBA

* Fix ``col_offset`` attribute for nodes involving ``with`` on ``PyPy``.

* Made ``FunctionDef.implicit_parameters`` return 1 for methods by making
``FunctionDef.is_bound`` return ``True``, as it does for class methods.

Closes PyCQA/pylint#6464

* Fixed a crash when ``_filter_stmts`` encounters an ``EmptyNode``.

Closes PyCQA/pylint#6438
Expand Down
5 changes: 4 additions & 1 deletion astroid/nodes/scoped_nodes/scoped_nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -1581,6 +1581,9 @@ def blockstart_tolineno(self):
"""
return self.args.tolineno

def implicit_parameters(self) -> Literal[0, 1]:
return 1 if self.is_bound() else 0

def block_range(self, lineno):
"""Get a range from the given line number to where this node ends.

Expand Down Expand Up @@ -1642,7 +1645,7 @@ def is_bound(self):
False otherwise.
:rtype: bool
"""
return self.type == "classmethod"
return self.type in {"method", "classmethod"}
Copy link
Member Author

Choose a reason for hiding this comment

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

This method was untested according to coveralls.


def is_abstract(self, pass_is_abstract=True, any_raise_is_abstract=False):
"""Check if the method is abstract.
Expand Down
18 changes: 17 additions & 1 deletion tests/unittest_brain_qt.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

@pytest.mark.skipif(HAS_PYQT6 is None, reason="This test requires the PyQt6 library.")
class TestBrainQt:
AstroidManager.brain["extension_package_whitelist"] = {"PyQt6"}

@staticmethod
def test_value_of_lambda_instance_attrs_is_list():
"""Regression test for https://github.com/PyCQA/pylint/issues/6221
Expand All @@ -28,11 +30,25 @@ def test_value_of_lambda_instance_attrs_is_list():
from PyQt6 import QtPrintSupport as printsupport
printsupport.QPrintPreviewDialog.paintRequested #@
"""
AstroidManager.brain["extension_package_whitelist"] = {"PyQt6.QtPrintSupport"}
node = extract_node(src)
attribute_node = node.inferred()[0]
if attribute_node is Uninferable:
pytest.skip("PyQt6 C bindings may not be installed?")
assert isinstance(attribute_node, UnboundMethod)
# scoped_nodes.Lambda.instance_attrs is typed as Dict[str, List[NodeNG]]
assert isinstance(attribute_node.instance_attrs["connect"][0], FunctionDef)

@staticmethod
def test_implicit_parameters() -> None:
"""Regression test for https://github.com/PyCQA/pylint/issues/6464"""
src = """
from PyQt6.QtCore import QTimer
timer = QTimer()
timer.timeout.connect #@
"""
node = extract_node(src)
attribute_node = node.inferred()[0]
if attribute_node is Uninferable:
pytest.skip("PyQt6 C bindings may not be installed?")
Comment on lines +51 to +52
Copy link
Member

Choose a reason for hiding this comment

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

Don't we want to force the c binding to work correctly in at least one of the CI pipeline ? Otherwise this test can work fine online and fail for someone with QT locally. I think we could create a pytest mark for "qt" and a CI job with a proper installation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it works in the "release tests" workflow.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, it means we're going to catch issues at release time right ? When/how are we launching this workflow ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just manually with a reminder in the release.md.

Installing qt takes 39s. Unless there's a way to cache it?

Copy link
Member

Choose a reason for hiding this comment

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

For 40s we might as well add it for each commit (one interpreter only ?). There's a lot of available runner for astroid and they run in a reasonable time (compared to pylint anyway).

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, it takes 8s, duh, because most of that linked run was setting up the env!

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused - why does "Install Qt" actually only do sudo apt-get install build-essential libgl1-mesa-dev? You are installing PyQt from binary wheels, so that 40s step can probably just be dropped entirely.

Copy link
Member Author

@jacobtylerwalls jacobtylerwalls Apr 28, 2022

Choose a reason for hiding this comment

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

We discovered that the PyQt6 wheel doesn't necessarily install the C bindings on ubuntu-latest, see failing run and comment.

assert isinstance(attribute_node, FunctionDef)
assert attribute_node.implicit_parameters() == 1
18 changes: 18 additions & 0 deletions tests/unittest_scoped_nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -617,6 +617,24 @@ def test():
self.assertIsInstance(one, nodes.Const)
self.assertEqual(one.value, 1)

def test_func_is_bound(self) -> None:
data = """
class MyClass:
def bound(): #@
pass
"""
func = builder.extract_node(data)
self.assertIs(func.is_bound(), True)
self.assertEqual(func.implicit_parameters(), 1)

data2 = """
def not_bound(): #@
pass
"""
func2 = builder.extract_node(data2)
self.assertIs(func2.is_bound(), False)
self.assertEqual(func2.implicit_parameters(), 0)

def test_type_builtin_descriptor_subclasses(self) -> None:
astroid = builder.parse(
"""
Expand Down