Skip to content

PR: Add QEnum macro for PyQt bindings#424

Merged
ccordoba12 merged 3 commits intospyder-ide:masterfrom
phil65:qenum
Aug 24, 2023
Merged

PR: Add QEnum macro for PyQt bindings#424
ccordoba12 merged 3 commits intospyder-ide:masterfrom
phil65:qenum

Conversation

@phil65
Copy link
Copy Markdown
Contributor

@phil65 phil65 commented Apr 8, 2023

synchronizes macro naming with PySide

@CAM-Gerlach CAM-Gerlach changed the title add QEnum macro for PyQt bindings PR: Add QEnum macro for PyQt bindings Apr 9, 2023
@CAM-Gerlach CAM-Gerlach added this to the v2.4.0 milestone Apr 9, 2023
@CAM-Gerlach
Copy link
Copy Markdown
Member

Thanks. Seems like Q_ENUM doesn't exist on PyQt 5.9, though, which we still support (due to Anaconda previously having been stuck on it until very recently). Is there an alternate name from which it can be imported on that version? Otherwise, you'll presumably want to handle it not being present (e.g. with getattr).

@dalthviz , thoughts?

@phil65
Copy link
Copy Markdown
Contributor Author

phil65 commented Apr 9, 2023

According to docs, it should have been added to Qt5.5 already. (https://doc.qt.io/qt-5/whatsnew55.html)
Perhaps there exists a Q_ENUMS macro instead of Q_ENUM for older python bindings?

@phil65 phil65 force-pushed the qenum branch 2 times, most recently from b6a9488 to f2097d1 Compare April 9, 2023 11:54
@phil65
Copy link
Copy Markdown
Contributor Author

phil65 commented Apr 9, 2023

Tried adding a fallback for 5.9.

EDIT: checks are passing now.

Copy link
Copy Markdown
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

LGTM from my side; thanks @phil65 . @dalthviz ?

@CAM-Gerlach CAM-Gerlach requested a review from dalthviz April 11, 2023 02:36
Copy link
Copy Markdown
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thank you for the work here @phil65 ! Could it be possible to add some tests for this?

Other than that this LGTM 👍

@CAM-Gerlach
Copy link
Copy Markdown
Member

To note, at least judging by what we've done in the past, it even can be pretty simple—even just an existence check on QEnum (e.g. assert QtCore.QEnum is not None). See the other tests for examples.

@phil65 phil65 force-pushed the qenum branch 4 times, most recently from 77e1ec9 to 43a60a2 Compare April 17, 2023 14:06
@dalthviz
Copy link
Copy Markdown
Member

Note: The errors in the tests come from pypa/pip#11975 and are unrelated to the changes done here

@dalthviz dalthviz closed this May 18, 2023
@dalthviz dalthviz reopened this May 18, 2023
@dalthviz
Copy link
Copy Markdown
Member

Note: Re-running the CI checks shows that there is an error with PySide2 (AttributeError: module 'qtpy.QtCore' has no attribute 'QEnum') 🤔

@phil65
Copy link
Copy Markdown
Contributor Author

phil65 commented May 24, 2023

Will take a look again when I find some time.

@flowln
Copy link
Copy Markdown

flowln commented Jul 15, 2023

Just a note here in case it helps, the QEnum macro was not added in PySide2 until 5.15 (https://bugreports.qt.io/browse/PYSIDE-957), so the tests will always fail on older Qt versions! :c

@dalthviz
Copy link
Copy Markdown
Member

Thank you @flowln for the feedback!

@phil65 I think what is missing then is adding a skip to the new test when PySide2 is being tested and has a version <5.15. Let us know if you need help with that!

@dalthviz
Copy link
Copy Markdown
Member

Hi @phil65 sorry for the sudden ping, just wondering, is it okay if I try to finish this? Let us know!

@dalthviz dalthviz changed the title PR: Add QEnum macro for PyQt bindings PR: Add QEnum macro for PyQt bindings Aug 11, 2023
@dalthviz
Copy link
Copy Markdown
Member

I will try to finish this so please @phil65 don't push any other commit to your branch :)

@dalthviz dalthviz self-assigned this Aug 16, 2023
Copy link
Copy Markdown
Member

@ccordoba12 ccordoba12 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, thanks @phil65!

@dalthviz
Copy link
Copy Markdown
Member

Is it okay if we merge this one @CAM-Gerlach ?

Copy link
Copy Markdown
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Checking the very minor changes since my last approval, they all LGTM, thanks—sorry for the delay!

@ccordoba12 ccordoba12 merged commit b5a8bb6 into spyder-ide:master Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants