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

Remove support for PySide and PyQt4 from CI #285

Merged
merged 4 commits into from
May 14, 2020
Merged

Conversation

mdickinson
Copy link
Member

Update the CI builds and etstool.py

  • Don't support PyQt4 and PySide; only PyQt5 and PySide2
  • Remove CI tests for PyQt4
  • Add CI tests for PyQt5 (from EDM) and PySide2 (from pip)

xref: https://github.com/enthought/pyface/pull/512/files

@mdickinson mdickinson requested a review from kitchoi May 13, 2020 18:21
"pyqt": {"pyqt<4.12"}, # FIXME: build of 4.12-1 appears to be bad
# XXX once pyqt5 is available in EDM, we will want it here
"pyqt5": set(),
"pyqt5": {"pyqt5"},
Copy link
Contributor

Choose a reason for hiding this comment

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

This mapping will be inconsistent with traitsui and pyface, where the mapping is "pyqt": {"pyqt5"} instead. It might add to the cognitive load for developers working across multiple ETS projects if envisage chooses a different naming.

I too preferred "pyqt5" in the toolkit name, but I deemed that opinion subjective. Afterall, this is only affecting developers' tools so this is easy to change one way or another.

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, that's a good point. I'll change it. But in that case, for consistency, we should probably be using pyside instead of pyside2. :-)

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, I think there is a good non-subjective reason to prefer pyqt5 over pyqt, which is that it matches the names of the corresponding EDM and PyPI packages.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to stick with the existing pyqt5, both because it matches the previous behaviour on this repo (I'd rather not switch pyqt from being valid and meaning one thing to still being valid and meaning something different) and because it matches the PyPI and EDM names.

@mdickinson mdickinson merged commit 45b43da into master May 14, 2020
@mdickinson mdickinson deleted the ci/update-qt-backends branch May 14, 2020 08:25
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