PR: Add missing `QtWebEngineWidgets.QWebEngineScript` import for PyQt5 and PySide2 by EasyIsrael · Pull Request #322 · spyder-ide/qtpy

EasyIsrael · 2022-02-26T00:14:52Z

No description provided.

CAM-Gerlach

For the other two, assuming it is available, it seems to make sense, but I'm unsure why this imports QWebEngineScript for PyQT6 and PYSIDE6, when its already imported by the import *.

The test failures on Python 3.10 are unrelated; I can look into fixing them—for some reason, the installed qtpy package isn't getting found on import in the tests.

CAM-Gerlach · 2022-02-26T01:28:44Z

qtpy/QtWebEngineWidgets.py

    from PyQt6.QtWebEngineCore import QWebEnginePage
    from PyQt6.QtWebEngineCore import QWebEngineSettings
    from PyQt6.QtWebEngineCore import QWebEngineProfile
+    from PyQt6.QtWebEngineWidgets import QWebEngineScript


I'm a little confused why this is one needed is. Isn't this name already imported by from PyQt6.QtWebEngineWidgets import *?

Suggested change

from PyQt6.QtWebEngineWidgets import QWebEngineScript

CAM-Gerlach · 2022-02-26T01:28:53Z

qtpy/QtWebEngineWidgets.py

    from PySide6.QtWebEngineCore import QWebEnginePage
    from PySide6.QtWebEngineCore import QWebEngineSettings
    from PySide6.QtWebEngineCore import QWebEngineProfile
+    from PySide6.QtWebEngineWidgets import QWebEngineScript


Same here...

Suggested change

from PySide6.QtWebEngineWidgets import QWebEngineScript

CAM-Gerlach · 2022-02-26T06:41:26Z

FYI, I've solved the CI issues in #324 ; once that's merged, you can git fetch upstream && git rebase upstream/master && git push -f to get the fix here.

EasyIsrael · 2022-02-26T13:51:50Z

I have no pyside6 and no pyqt6 installed, so i just tested it on pyside2 and pyqt5. Those two work now with my changes. For the others i just added it the same way, because i thought it is neccesary. CAM Gerlach ***@***.***> schrieb am Sa., 26. Feb. 2022, 02:41:

…

***@***.**** requested changes on this pull request. For the other two, assuming it is available, it seems to make sense, but I'm unsure why this imports QWebEngineScript for PyQT6 and PYSIDE6, when its already imported by the import *. The test failures on Python 3.10 are unrelated; I can look into fixing them—for some reason, the installed qtpy package isn't getting found on import in the tests. ------------------------------ In qtpy/QtWebEngineWidgets.py <#322 (comment)>: > @@ -33,15 +34,20 @@ from PyQt6.QtWebEngineCore import QWebEnginePage from PyQt6.QtWebEngineCore import QWebEngineSettings from PyQt6.QtWebEngineCore import QWebEngineProfile + from PyQt6.QtWebEngineWidgets import QWebEngineScript I'm a little confused why this is one needed is. Isn't this name already imported by from PyQt6.QtWebEngineWidgets import *? ------------------------------ In qtpy/QtWebEngineWidgets.py <#322 (comment)>: > elif PYSIDE6: from PySide6.QtWebEngineWidgets import * from PySide6.QtWebEngineCore import QWebEnginePage from PySide6.QtWebEngineCore import QWebEngineSettings from PySide6.QtWebEngineCore import QWebEngineProfile + from PySide6.QtWebEngineWidgets import QWebEngineScript Same here... — Reply to this email directly, view it on GitHub <#322 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXHBXMUX4KPCG5LUR72IULU5AVV7ANCNFSM5PLTBIBA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>

CAM-Gerlach · 2022-02-26T16:36:21Z

For the others i just added it the same way, because i thought it is necessary.

Actually, it isn't—they already work with those bindings, because right above where they were added, QtPy has the lines

from PyQt6.QtWebEngineWidgets import *

and

from PySide6.QtWebEngineWidgets import *

This imports all top-level non-private names from the QtWebEngineWidgets module, including QWebEngineScript.

Therefore, those two lines added can be removed, as I've made GitHub suggestions for (you can either batch-apply them, or do so manually).

…ssion PR: Restrict broken Pytest versions to those not affected by the Pytest 7.0.0 import-mode=importlib behavior regression

dalthviz

Thank you for your help with this @EasyIsrael ! Already merged the PR to fix our tests (#324) so you can go ahead and rebase to get the changes 👍 Also, besides @CAM-Gerlach comments regarding the removal of some lines added on PyQt6 and PySide6, it would be nice to add a new assertion when testing the module here:

qtpy/qtpy/tests/test_qtwebenginewidgets.py

Lines 4 to 12 in 1cd704f

    
           @pytest.mark.skipif(PYSIDE6 or PYQT6, reason="Only available in Qt<6,>=6.2 bindings") 
        
           def test_qtwebenginewidgets(): 
        
               """Test the qtpy.QtWebEngineWidget namespace""" 
        
               QtWebEngineWidgets = pytest.importorskip("qtpy.QtWebEngineWidgets") 
        
               assert QtWebEngineWidgets.QWebEnginePage is not None 
        
               assert QtWebEngineWidgets.QWebEngineView is not None 
        
               assert QtWebEngineWidgets.QWebEngineSettings is not None

So adding the a line like:

assert QtWebEngineWidgets.QWebEngineScript is not None

Let us know if you have any question or if you need help with the rebase or to do the changes

CAM-Gerlach · 2022-03-01T04:55:59Z

(As a reminder, the commands to rebase this:)

git fetch upstream
git rebase upstream/master
git push -f

CAM-Gerlach · 2022-03-01T18:33:04Z

Hey @EasyIsrael, thanks for bearing with us! It looks like things went wrong with Git here—seems like the original issue, which I should have noticed before, is that the PR was submitted from your master branch, rather than a feature branch with the changes. As a consequence things went haywire when you tried to pull in the CI fixes, with my changes are showing up on your PR, while the commit history shows multiple merges and the same commits multiple times, which not only is an issue here, but also will cause even more issues for you next time you try to submit a PR.

While you could cherry-pick the commits, since its only two lines in one file being changed, its probably easier to just copy them and redo them. So what I suggest to fix your repo is:

Copy (or just remember) the two lines you've changed.

Run the following to fix your master branch locally and remotely and set up a new feature branch:

git fetch --all
git reset --hard upstream/master
git push -f
git checkout -b add-qwebenginescript

Paste/type in the two changed lines

Run the following to commit and push (as normal):

git commit -am "Add missing QWebEngineScript import for PyQt5 and PySide2"
git push -u origin add-qwebenginescript

Create a new PR here off that branch and close this one

Thanks!

EasyIsrael · 2022-03-01T21:18:16Z

Thank you CAM-Gerlach and dalthviz for your help
to push my changes to the qtpy repository.
It seems to be a bit more complicated. I'm not so familiar with git and the branch stuff. I will try to copy n paste my changes to the original master branch and make another pull request. I hope that works then.

CAM-Gerlach · 2022-03-01T23:33:33Z

Thanks, @EasyIsrael , looks like it did, so I'll close this one.

QWebEngineScript added

1cd704f

CAM-Gerlach requested changes Feb 26, 2022

View reviewed changes

Disable --importmode=importlib to work around pytest-dev/pytest#9645

d9dc0be

Merge pull request spyder-ide#324 from CAM-Gerlach/fix-pytest-7-regre…

24fbb72

…ssion PR: Restrict broken Pytest versions to those not affected by the Pytest 7.0.0 import-mode=importlib behavior regression

dalthviz reviewed Feb 28, 2022

View reviewed changes

dalthviz changed the title ~~QWebEngineScript added~~ PR: Add missing QtWebEngineWidgets.QWebEngineScript import for PyQt5 and PySide2 Feb 28, 2022

dalthviz assigned EasyIsrael Feb 28, 2022

dalthviz added this to the v2.1.0 milestone Feb 28, 2022

dalthviz added the type:Enhancement label Feb 28, 2022

EasyIsrael added 3 commits March 1, 2022 15:17

QWebEngineScript added

77fcf41

deleted unnecessary import for pyside6 and pyqt6

0ad5c9d

Merge branch 'master' of https://github.com/EasyIsrael/qtpy

6a9a52f

CAM-Gerlach marked this pull request as draft March 1, 2022 17:21

CAM-Gerlach closed this Mar 1, 2022

ccordoba12 removed this from the v2.1.0 milestone Mar 1, 2022

CAM-Gerlach mentioned this pull request Mar 2, 2022

PR: Add missing QWebEngineScript support for PyQt5/6 and PySide2/6 #326

Merged

	@pytest.mark.skipif(PYSIDE6 or PYQT6, reason="Only available in Qt<6,>=6.2 bindings")
	def test_qtwebenginewidgets():
	"""Test the qtpy.QtWebEngineWidget namespace"""

	QtWebEngineWidgets = pytest.importorskip("qtpy.QtWebEngineWidgets")

	assert QtWebEngineWidgets.QWebEnginePage is not None
	assert QtWebEngineWidgets.QWebEngineView is not None
	assert QtWebEngineWidgets.QWebEngineSettings is not None

Uh oh!

Conversation

EasyIsrael commented Feb 26, 2022

Uh oh!

CAM-Gerlach left a comment

Choose a reason for hiding this comment

Uh oh!

CAM-Gerlach Feb 26, 2022

Choose a reason for hiding this comment

Uh oh!

CAM-Gerlach Feb 26, 2022

Choose a reason for hiding this comment

Uh oh!

CAM-Gerlach Feb 26, 2022

Choose a reason for hiding this comment

Uh oh!

CAM-Gerlach Feb 26, 2022

Choose a reason for hiding this comment

Uh oh!

CAM-Gerlach commented Feb 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EasyIsrael commented Feb 26, 2022 via email

Uh oh!

CAM-Gerlach commented Feb 26, 2022

Uh oh!

dalthviz left a comment

Choose a reason for hiding this comment

Uh oh!

CAM-Gerlach commented Mar 1, 2022

Uh oh!

CAM-Gerlach commented Mar 1, 2022

Uh oh!

EasyIsrael commented Mar 1, 2022

Uh oh!

CAM-Gerlach commented Mar 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CAM-Gerlach commented Feb 26, 2022 •

edited

Loading