Skip to content

PR: Fix LibraryLocation -> LibraryPath renaming due to deprecation with Qt6#393

Merged
dalthviz merged 2 commits intospyder-ide:masterfrom
StSav012:patch-1
Dec 19, 2022
Merged

PR: Fix LibraryLocation -> LibraryPath renaming due to deprecation with Qt6#393
dalthviz merged 2 commits intospyder-ide:masterfrom
StSav012:patch-1

Conversation

@StSav012
Copy link
Copy Markdown
Contributor

@StSav012 StSav012 commented Dec 17, 2022

That was a minor omission.

@CAM-Gerlach CAM-Gerlach changed the title Fixed LibraryLocation → LibraryPath renaming Fix LibraryLocation -> LibraryPath renaming Dec 18, 2022
@CAM-Gerlach CAM-Gerlach added this to the v2.3.1 milestone Dec 18, 2022
@CAM-Gerlach CAM-Gerlach requested a review from dalthviz December 18, 2022 00:51
@StSav012
Copy link
Copy Markdown
Contributor Author

StSav012 commented Dec 18, 2022

More info on the change: https://doc.qt.io/qt-6/qlibraryinfo-obsolete.html

@dalthviz dalthviz changed the title Fix LibraryLocation -> LibraryPath renaming PR: Fix LibraryLocation -> LibraryPath renaming due to deprecation with Qt6 Dec 18, 2022
dalthviz
dalthviz reviewed Dec 18, 2022
View reviewed changes
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.

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!

@StSav012
Copy link
Copy Markdown
Contributor Author

StSav012 commented Dec 19, 2022

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

QtCore.QTextStreamManipulator.exec_ is not None
in the tests correct? I would expect an 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 asserts aren't necessary at all.

@dalthviz
Copy link
Copy Markdown
Member

dalthviz commented Dec 19, 2022

in the tests correct? I would expect an 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 asserts 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 :)

dalthviz
dalthviz approved these changes Dec 19, 2022
View reviewed changes
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.

Thanks @StSav012 !

@dalthviz dalthviz merged commit 1bb248a into spyder-ide:master Dec 19, 2022
@StSav012 StSav012 mentioned this pull request Apr 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dalthviz dalthviz dalthviz approved these changes

Assignees

@StSav012 StSav012

Labels

type:Bug

Projects

None yet

Milestone

v2.3.1

Development

Successfully merging this pull request may close these issues.

3 participants

@StSav012 @dalthviz @CAM-Gerlach