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

Add pyi stubs that redirect to PySide2 #371

Merged
merged 5 commits into from
Feb 13, 2023
Merged

Conversation

chadrik
Copy link
Contributor

@chadrik chadrik commented Aug 22, 2022

Since there are a number of competing stubs for PySide out there (my contribution is here), and someone may prefer to pip install one stub package over another, I propose creating a set of .pyi files that defer to PySide2, since that's the layout that Qt.py standardizes everything to.

After this change, installing Qt.py with type support would be done like this:

pip install Qt.py types-PySide2

or

pip install Qt.py[stubs]

@CLAassistant
Copy link

CLAassistant commented Aug 22, 2022

CLA assistant check
All committers have signed the CLA.

@chadrik
Copy link
Contributor Author

chadrik commented Sep 19, 2022

I think this is all ready to go. The build jobs are failing, but they seem unrelated. Any suggestions?

@mottosso
Copy link
Owner

Hey, sorry for the delay.

I had a look at the breaking tests, which led me to take a closer look at the changes in this PR, and it's turned Qt.py from a module to a package! That's not something we want to do; one of the USPs of Qt.py is that it is a self-contained single file. Once we head down the package route we'll introduce a series of other questions too, like why not separate QtCore, QtWidgets etc. into separate modules too?

Is there any way of achieving the goal of these stubs without having any effect on Qt.py the module?

@chadrik
Copy link
Contributor Author

chadrik commented Sep 26, 2022

That's my bad, I should have known better than to make a change that drastic.

pep 561 doesn't directly discuss how to handle a module whose stubs are a package, so what I've done is to structure the stubs as a "stubs only" package, which is intended to be distributed as a separately installable package, but there's nothing stopping us distributing both module and stubs-only package from a single wheel. I built and tested this and it works.

Here's how I tested:

python3 setup.py bdist_wheel
mkdir stubstest
cd stubstest
python3 venv .venv
. .venv/bin/activate
pip install mypy types-PySide2 ../dist/Qt.py-1.3.7-py2.py3-none-any.whl
echo $'from Qt import QtCore, QtWidgets\nw = QtWidgets.QWidget()' > qtstubtest.py
mypy qtstubtest.py

For the record, I tried making an all-in-one Qt.pyi file, but I can't use a single module to properly define the Qt.Qt sub-module, which imports the contents of numerous other modules.

@chadrik
Copy link
Contributor Author

chadrik commented Sep 26, 2022

I also added the feature to easily install PySide2 stubs via pip install Qt.py[stubs]

@chadrik
Copy link
Contributor Author

chadrik commented Sep 26, 2022

One more thought here:

I think we should add types-PySide2 as a default requirement, and here's why:

  • I added something to the readme explaining how to get code completion working (pip install Qt.py[stubs]) but the reality is that 99% of people who experience broken code completion for Qt.py will not read this documentation, they'll simply be frustrated that it doesn't work out of the box.
  • types-PySide2 does not have any impact on runtime behavior: it's just a package of .pyi files. Qt.py will not generate any errors if the Qt-stubs or types-PySide2 packages are missing. These stub packages exist solely for code completion and static analysis.
  • Those people who are pip installing their packages have already accepted the added complexity of a virtual environment and adding types-PySide2 isn't going to add any additional complexity or issues.
  • Those who install Qt.py as a simple drop-in module continue to use exactly the same workflow. Adding this new requirement doesn't affect that workflow at all.

Ultimately it doesn't matter to me that much, but I thought I'd put this info out there so that you can make the call. Let me know what you'd like to do.

@mottosso
Copy link
Owner

Thanks, I can see what you mean and I agree.

Advanced users can still copy/paste the source of Qt.py into their own projects which is how I normally use it. And that those who don't care for or would use it still get auto-completion during a pip install seems fine; does no harm.

I was going to suggest we add tests for this; such that we know that when Travis is happy, the auto-completion mechanic is working as intended. But, thinking about it, I'm not sure how to test for it? 🤔 If we can't then fair enough. If you're happy with the functionality, and preferably one more third-party - such as @fredrikaverpil, @matiascodesal or @MaximeEngel - then I'm happy to merge this.

@chadrik
Copy link
Contributor Author

chadrik commented Sep 26, 2022

I was going to suggest we add tests for this

I was hoping to do this as well. It looks like all of the test machinery is currently built around testing the Qt.py module "in situ" so it would require some reworking, because I think that the Qt-stubs package will only work if it's been installed into a site-packages directory. The best way to to test this is to build a wheel of Qt.py, pip install it into a virtualenv, and run mypy (we will need to restrict it to a version that's compatible with python 3.6).

Tox would be a good candidate for test runner, since by default tox pip installs your package into a tmp venv before running the test commands, which ensures that your setup.py is working and installs the other specified requirements. Tox is higher level than nose or pytest: it is designed to run multiple test commands with differing entry points, requirements, python versions, etc. So we would convert these tests into tox "environments":

printf "#\n# Running tests in Python ${PYTHON}\n"
export NOSETESTS_BINARY=nosetests${PYTHON}
printf "#\n# Testing implementation..\n"
    python${PYTHON} -u run_tests.py
printf "#\n# Testing caveats..\n"
    python${PYTHON} build_caveats.py
    nosetests${PYTHON} \
        --verbose \
        --with-doctest \
        --with-process-isolation \
        test_caveats.py
printf "#\n# Testing examples..\n"
    nosetests${PYTHON} \
    --verbose \
    --with-process-isolation \
    --with-doctest \
    --exe \
        examples/*/*.py

printf Done

Then we would add a new one with a mypy test.

I'm a little wary of wading into this, since it won't be a small project.

@fredrikaverpil
Copy link
Collaborator

fredrikaverpil commented Sep 29, 2022

Hi guys, 👋 long time 😄

I haven't used Qt.py in projects for several years now, so I'm not fully in the loop here. But with the history of Qt.py being a "standalone" module and easy to vendor I think your solution is great @chadrik. However, I would personally not use setup.py anymore now that pyproject.toml supports all of this out of the box - and removes all the remote code execution risks that comes with setup.py. But that's out of scope for this PR.

Adding working completion will bring a lot of value to end users and I think it's valuable to merge this in even if it does not contain tests. @mottosso hacktoberfest is coming up, maybe adding tests for this could be something to request from people seeking to contribute?
But I'm not entirely sure how to test this and what to include in the scope. Maybe it would be sufficient to just verify that the intended functionality even works at all by verifying completion works for one function call. Like a minimal sanity check.

Regardless, Qt.py has always relied partly on user input on missing or wrong PySide2 attributes/methods. I think this will partly help keep these stubs updated.

Even if I'm really passive in this project these days, and for whatever it's worth, I'm all in favour for merging this in as I can't imagine coding "in the dark" like I used to do back in 2016 🥲
Nice work @chadrik!

@mottosso
Copy link
Owner

as I can't imagine coding "in the dark" like I used to do back in 2016

Ouch! 😅 Thanks for sharing your thoughts!

hacktoberfest is coming up, maybe adding tests for this could be something to request from people seeking to contribute?

Interesting, I'm not in the loop on that. Would you be able to organise?

@fredrikaverpil
Copy link
Collaborator

fredrikaverpil commented Sep 29, 2022

Just tag the repo with the hacktoberfest topic.

You can actively register the repo at https://hacktoberfest.com and formally register the repo as "eligible" or even register to host an event.

  • Register anytime between September 26 and October 31

  • Pull requests can be made in any GITHUB or GITLAB hosted project that’s participating in Hacktoberfest (look for the “hacktoberfest” topic)

  • Project maintainers must accept your pull/merge requests for them to count toward your total

  • Have 4 pull/merge requests accepted between October 1 and October 31 to complete Hacktoberfest

  • The first 40,000 participants (maintainers and contributors) who complete Hacktoberfest can elect to receive one of two prizes: a tree planted in their name, or the Hacktoberfest 2022 t-shirt.

-- https://hacktoberfest.com/participation/

I don't feel active around here to do any of this though (there might be follow-up questions), sorry.

@chadrik
Copy link
Contributor Author

chadrik commented Sep 29, 2022

Ok, I pushed up the change to make types-PySide2 an install requirement. Let me know if you want me to squash my commits.

Since this is a big packaging change it might be good to push to test.pypi.org first and see if folks on the original thread want to try it out.

as I can't imagine coding "in the dark" like I used to do back in 2016

Ouch! 😅 Thanks for sharing your thoughts!

I think Fredrik is referring to coding without type stubs/annotations. Correct?

However, I would personally not use setup.py anymore now that pyproject.toml supports all of this out of the box - and removes all the remote code execution risks that comes with setup.py.

pyproject.toml has its benefits, but the code execution risks are already mitigated by the use of wheels.

But that's out of scope for this PR.

agreed.

@chadrik
Copy link
Contributor Author

chadrik commented Sep 29, 2022

hacktoberfest is coming up, maybe adding tests for this could be something to request from people seeking to contribute?

I think it would be simpler if you framed the task as porting the tests to tox -- and addressing how that affects the docker containers and the entrypoint script. Once that's done I'd be happy to add a mypy test, as it'll be a 30 minute fix for someone who knows their way around tox and mypy.

@fredrikaverpil
Copy link
Collaborator

I think Fredrik is referring to coding without type stubs/annotations. Correct?

👍 😄

@mottosso
Copy link
Owner

I think Fredrik is referring to coding without type stubs/annotations. Correct?

Haha, yes. I'm the one in the dark ages still.

@chadrik
Copy link
Contributor Author

chadrik commented Feb 12, 2023

Hi all, just checking in on this. Shall we get it merged? I feel it's pretty safe.

@mottosso
Copy link
Owner

mottosso commented Feb 13, 2023

I had a quick final check on my machine with regards to the added stubs dependency, and it seems safe.

(qttemp) PS C:\Users\marcus> pip install https://github.com/chadrik/Qt.py/archive/refs/heads/pyi-stubs.zip
Collecting https://github.com/chadrik/Qt.py/archive/refs/heads/pyi-stubs.zip
  Using cached https://github.com/chadrik/Qt.py/archive/refs/heads/pyi-stubs.zip
Collecting types-PySide2
  Using cached types_PySide2-5.15.2.1.2-py3-none-any.whl (577 kB)
Using legacy 'setup.py install' for Qt.py, since package 'wheel' is not installed.
Installing collected packages: types-PySide2, Qt.py
    Running setup.py install for Qt.py ... done
Successfully installed Qt.py-1.3.7 types-PySide2-5.15.2.1.2
(qttemp) PS C:\Users\marcus>

Merging this and making a new release. Thanks @chadrik and @fredrikaverpil! 🥳

EDIT: https://github.com/mottosso/Qt.py/releases/tag/1.3.8

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.

4 participants