Skip to content

qt brain: Make slot argument optional for disconnect()#1550

Merged
Pierre-Sassoulas merged 3 commits intopylint-dev:mainfrom
The-Compiler:qt-none-slot
May 12, 2022
Merged

qt brain: Make slot argument optional for disconnect()#1550
Pierre-Sassoulas merged 3 commits intopylint-dev:mainfrom
The-Compiler:qt-none-slot

Conversation

@The-Compiler
Copy link
Copy Markdown
Contributor

@The-Compiler The-Compiler commented May 10, 2022 •
edited
Loading

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.

If this is an acceptable fix, I'll write a changelog - two more questions:

  • Is there a good way I can write a test for this?
  • Should this go to main or to a maintenance branch

Description

Discussed here:
#1531 (comment)

PyQt supports calling .disconnect() without any arguments in order to
disconnect all slots:
https://www.riverbankcomputing.com/static/Docs/PyQt6/signals_slots.html#disconnect

Strictly speaking, slot=None is a wrong call, as actually passing None
does not work:
python-qt-tools/PyQt5-stubs#103

However, pylint/astroid does not support overloads needed to properly
express this: pylint-dev/pylint#5264

So, while this is "wrong", it's less wrong than before - without this
change, pylint expects a mandatory argument, thus raising a
false-positive here:

from PyQt5.QtCore import QTimer
t = QTimer()
t.timeout.connect(lambda: None)
t.timeout.disconnect()

despite running fine, pylint complains:

test.py:4:0: E1120: No value for argument 'slot' in method call (no-value-for-parameter)

while with this change, things work fine.

Type of Changes

Type
✓ 🐛 Bug fix

@The-Compiler
qt brain: Make slot argument optional for disconnect()
f5ec7ac 
Discussed here:
#1531 (comment)

PyQt supports calling .disconnect() without any arguments in order to
disconnect all slots:
https://www.riverbankcomputing.com/static/Docs/PyQt6/signals_slots.html#disconnect

Strictly speaking, slot=None is a wrong call, as actually passing None
does not work:
python-qt-tools/PyQt5-stubs#103

However, pylint/astroid does not support overloads needed to properly
express this: pylint-dev/pylint#5264

So, while this is "wrong", it's less wrong than before - without this
change, pylint expects a mandatory argument, thus raising a
false-positive here:

    from PyQt5.QtCore import QTimer
    t = QTimer()
    t.timeout.connect(lambda: None)
    t.timeout.disconnect()

despite running fine, pylint complains:

    test.py:4:0: E1120: No value for argument 'slot' in method call (no-value-for-parameter)

while with this change, things work fine.
@Pierre-Sassoulas Pierre-Sassoulas added the Brain 🧠 Needs a brain tip label May 10, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.12.0 milestone May 10, 2022
@coveralls
Copy link
Copy Markdown

coveralls commented May 10, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 2311757594

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 143 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.02%) to 91.665%
Files with Coverage Reduction New Missed Lines %
astroid/brain/brain_typing.py 10 88.03%
astroid/rebuilder.py 17 96.97%
astroid/brain/brain_builtin_inference.py 26 90.99%
astroid/nodes/scoped_nodes/scoped_nodes.py 90 92.48%
Totals Coverage Status
Change from base Build 2299632152: 0.02%
Covered Lines: 9139
Relevant Lines: 9970
💛 - Coveralls

@Pierre-Sassoulas
Copy link
Copy Markdown
Member

Pierre-Sassoulas commented May 10, 2022

Is there a good way I can write a test for this?

Check unittest_brain_qt.py there's a pytest mark for this: https://github.com/PyCQA/astroid/blob/main/tests/unittest_brain_qt.py#L17

Should this go to main or to a maintenance branch

Let's put it on the main, then we can decide if this need to be backported to the maintenance branche or not :)

@The-Compiler
Copy link
Copy Markdown
Contributor Author

The-Compiler commented May 10, 2022

Check unittest_brain_qt.py there's a pytest mark for this: ...

I've seen that file and tried to write a test inspired by test_implicit_parameters, but I'm lost about checking that it's valid to call this function without any parameters (or somehow checking that this FunctionDef has no mandatory args).

As for the fix itself: One thing we could do to avoid implying that None is okay is to use a _UNSET = object() sentinel as default value instead. I just updated the code accordingly.

Let's put it on the main, then we can decide if this need to be backported to the maintenance branche or not :)

How does the changelog entry work? Do I add it to the earlier version here in the main branch?

The-Compiler added a commit to qutebrowser/qutebrowser that referenced this pull request May 10, 2022
@The-Compiler
Update pylint ignores
b6c16f1 
See pylint-dev/astroid#1550 for the signal
disconnect.

Not reporting the gen_classes() one, as we have another ignore for that,
and doing something a bit unorthodox there anyways.
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.12.0, 2.11.6 May 10, 2022
@Pierre-Sassoulas
Copy link
Copy Markdown
Member

Pierre-Sassoulas commented May 10, 2022

How does the changelog entry work? Do I add it to the earlier version here in the main branch?

Yes in the 2.11.6 section

@jacobtylerwalls
Copy link
Copy Markdown
Member

jacobtylerwalls commented May 10, 2022

Thanks for the fix!

I've seen that file and tried to write a test inspired by test_implicit_parameters, but I'm lost about checking that it's valid to call this function without any parameters (or somehow checking that this FunctionDef has no mandatory args).

You should be able to follow that test to extract the node for the FunctionDef representing disconnect(), and then drill into its .args.kwargs and assert something. The #@ symbol in the code block is for jumping straight to the node you want to extract.

@The-Compiler
Copy link
Copy Markdown
Contributor Author

The-Compiler commented May 12, 2022

Updated with a test and changelog now. Not too happy with the test since it basically 1:1 checks what the implementation is, but I guess there is not much more we can do if the test should live here and not in pylint.

@Pierre-Sassoulas
Copy link
Copy Markdown
Member

Pierre-Sassoulas commented May 12, 2022

Don't mind the failing test, it's due to #1551

@jacobtylerwalls jacobtylerwalls changed the title RFC: qt brain: Make slot argument optional for disconnect() qt brain: Make slot argument optional for disconnect() May 12, 2022
Pierre-Sassoulas
Pierre-Sassoulas approved these changes May 12, 2022
View reviewed changes
Copy link
Copy Markdown
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.

👍

@Pierre-Sassoulas Pierre-Sassoulas merged commit 5aa0f51 into pylint-dev:main May 12, 2022
@The-Compiler The-Compiler mentioned this pull request May 18, 2022
Merged
Pierre-Sassoulas pushed a commit that referenced this pull request Jun 13, 2022
@The-Compiler @Pierre-Sassoulas
qt brain: Make slot argument optional for disconnect() (#1550)
da9df99 
* qt brain: Make slot argument optional for disconnect()

Discussed here:
#1531 (comment)

PyQt supports calling .disconnect() without any arguments in order to
disconnect all slots:
https://www.riverbankcomputing.com/static/Docs/PyQt6/signals_slots.html#disconnect

Strictly speaking, slot=None is a wrong call, as actually passing None
does not work:
python-qt-tools/PyQt5-stubs#103

However, pylint/astroid does not support overloads needed to properly
express this: pylint-dev/pylint#5264

So, while this is "wrong", it's less wrong than before - without this
change, pylint expects a mandatory argument, thus raising a
false-positive here:

    from PyQt5.QtCore import QTimer
    t = QTimer()
    t.timeout.connect(lambda: None)
    t.timeout.disconnect()

despite running fine, pylint complains:

    test.py:4:0: E1120: No value for argument 'slot' in method call (no-value-for-parameter)

while with this change, things work fine.
Pierre-Sassoulas pushed a commit that referenced this pull request Jun 13, 2022
@The-Compiler @Pierre-Sassoulas
qt brain: Make slot argument optional for disconnect() (#1550)
b30977e 
* qt brain: Make slot argument optional for disconnect()

Discussed here:
#1531 (comment)

PyQt supports calling .disconnect() without any arguments in order to
disconnect all slots:
https://www.riverbankcomputing.com/static/Docs/PyQt6/signals_slots.html#disconnect

Strictly speaking, slot=None is a wrong call, as actually passing None
does not work:
python-qt-tools/PyQt5-stubs#103

However, pylint/astroid does not support overloads needed to properly
express this: pylint-dev/pylint#5264

So, while this is "wrong", it's less wrong than before - without this
change, pylint expects a mandatory argument, thus raising a
false-positive here:

    from PyQt5.QtCore import QTimer
    t = QTimer()
    t.timeout.connect(lambda: None)
    t.timeout.disconnect()

despite running fine, pylint complains:

    test.py:4:0: E1120: No value for argument 'slot' in method call (no-value-for-parameter)

while with this change, things work fine.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas approved these changes

@jacobtylerwalls jacobtylerwalls jacobtylerwalls approved these changes

Assignees

No one assigned

Labels

Brain 🧠 Needs a brain tip

Projects

None yet

Milestone

2.11.6

Development

Successfully merging this pull request may close these issues.

4 participants

@The-Compiler @coveralls @Pierre-Sassoulas @jacobtylerwalls