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

Improve bound and unbound signal hints #56

Merged
merged 19 commits into from
Oct 1, 2020

Conversation

altendky
Copy link
Collaborator

@altendky altendky commented Sep 12, 2020

pyqtSignal is a descriptor and does not have emit, connect, and disconnect methods. Rather it is used as a class attribute and when you access an_instance.the_signal you get a pyqtBoundSignal instead of the original pyqtSignal. So the bound signal needs those methods and the unbound signal needs .__get__().

Draft for:

  • Maybe we want to get the build passing before making big changes?
  • I should look at adding tests related to this.
    • They are pretty minimal, but something.

`pyqtSignal` is a descriptor and does not have `emit`, `connect`, and `disconnect` methods.  Rather it is used as a class attribute and when you access `an_instance.the_signal` you get a `pyqtBoundSignal` instead of the original `pyqtSignal`.  So the bound signal needs those methods and the unbound signal needs `__get__()`.
@altendky altendky marked this pull request as draft September 12, 2020 19:14
@altendky
Copy link
Collaborator Author

@altendky
Copy link
Collaborator Author

@stlehmann, @BryceBeagle, aside from whether this is correct at the moment or not (still need to add tests etc) but would you rather take signal improvements piecemeal or try to work through a 'complete' solution from the get go? Basically, work through this or just go all out and come up with a solution for #68? I would personally default to taking multiple steps since I'm not sure what the whole solution is. :]

@BryceBeagle
Copy link
Collaborator

I think piecemeal is probably better, so changes can keep rolling out. Functionality is never lost by doing this.

We could make a meta tracking issue if we feel there are many signal-based features to implement.

@altendky altendky marked this pull request as ready for review October 1, 2020 03:31
Copy link
Collaborator

@BryceBeagle BryceBeagle left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just a small question that probably won't affect approval

@@ -31,4 +31,4 @@ def test_files(filename):
path = os.path.join(TESTS_DIR, filename)
with open(path, 'r') as f:
code = f.read()
exec(compile(code, filename, 'exec'))
exec(compile(code, filename, 'exec'), {})
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the purpose of setting globals arg?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It complained that the name QtCore didn't exist without it. Which is odd. Worked fine in a REPL as it was but not in pytest. But mostly I don't understand why things are done this way vs. regular old tests. I expected at some point to take a pass at revamping the testing but if you can explain the point of this setup with exec now, by all means go for it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope I have no clue why it's done this way haha

@altendky altendky merged commit c764d7c into python-qt-tools:master Oct 1, 2020
@altendky altendky deleted the patch-2 branch October 1, 2020 17:47
bluebird75 pushed a commit to bluebird75/PyQt5-stubs that referenced this pull request Aug 24, 2021
Improve bound and unbound signal hints
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.

2 participants