-
-
Notifications
You must be signed in to change notification settings - Fork 153
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
PR: Fix LibraryLocation
-> LibraryPath
renaming due to deprecation with Qt6
#393
Conversation
More info on the change: https://doc.qt.io/qt-6/qlibraryinfo-obsolete.html |
LibraryLocation
-> LibraryPath
renaming due to deprecation with Qt6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @StSav012 thank you for the help with this! I think we should add a test for this. It should go inside test_qtcore.py
and be something similar to the tests we have for QLibraryInfo.location
:
qtpy/qtpy/tests/test_qtcore.py
Lines 65 to 68 in 371ec52
def test_qlibraryinfo_location(): | |
"""Test QLibraryInfo.location""" | |
assert QtCore.QLibraryInfo.location is not None | |
assert QtCore.QLibraryInfo.location(QtCore.QLibraryInfo.PrefixPath) is not None |
Other than that, this LGTM 👍 If you have any question to implement the test let us know!
Hi @dalthviz, Thank you for the feedback. Forgive me for not looking through the tests, for I'm not disciplines enough (yet) to make tests for my code. As a side note, is line qtpy/qtpy/tests/test_qtcore.py Line 73 in 371ec52
assert there, like you have in similar places. On the other hand, if there is no exec_ function, the expression should raise an AttributeError , right? Then, many assert s aren't necessary at all.
|
You are probably right there @StSav012 ! In general I believe we have a lot of room for improvement regarding testing 😅 If you want to help with that or have any ideas feel free to open a new issue to discuss and maybe plan what changes we could do to improve our test suite :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @StSav012 !
That was a minor omission.