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

PR: Fix bug when importing from PySide6.QtWebEngineCore/QtWebEngineWidgets (QWebEngineScrip vs QWebEngineScript) #455

Merged
merged 3 commits into from Sep 7, 2023
Merged

Conversation

damonlynch
Copy link
Contributor

Fix typo in import statement

@dalthviz dalthviz added this to the v2.4.1 milestone Sep 4, 2023
Copy link
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 @damonlynch for spotting this error and submitting a fix! Could it be possible for you to also remove or modify the test skip we have at

@pytest.mark.skipif(
PYSIDE6 or PYQT6,
reason="Only available in Qt<6,>=6.2 bindings",
)

?

Other than that this LGTM 👍 Let us know If you need any help regarding the test modification or if it is okay for us to do that change here!

@dalthviz dalthviz changed the title Fix bug when importing from PySide6.QtWebEngineCore PR: Fix bug when importing from PySide6.QtWebEngineCore (QWebEngineScrip vs QWebEngineScript) Sep 4, 2023
@damonlynch
Copy link
Contributor Author

Thanks @dalthviz, I have made the changes to the test code, which were pretty simple. However, my knowledge of git is quite minimal. Should I make a new, separate commit with the changes, or use some other kind of git command to incorporate the changes? Then how do I incorporate the changes into this pull request?

By the way, I was unable to understand how to set up pytest using the instructions in CONTRIBUTING.md. I ran the command pre-commit install, but I have no idea if that installed pytest.

@dalthviz
Copy link
Member

dalthviz commented Sep 5, 2023

However, my knowledge of git is quite minimal. Should I make a new, separate commit with the changes, or use some other kind of git command to incorporate the changes? Then how do I incorporate the changes into this pull request?

No problem! So, you will need to do the test change and commit that change over the fork branch that you named PySide6-QWebEngineScript-fix (that's the branch on your fork connected to this pull request). For that, if you already cloned your fork repo in your machine, run from a terminal or git bash instance something like:

cd <path where your fork clone is located>
git checkout PySide6-QWebEngineScript-fix

Then, update the relevant test file, add the changes, create a commit and push it. After doing the change to the test file, you will need to run something like:

git add .
git commit -m "Update QtWebEngineCore test"
git push

If that sounds a little bit to complicated, I think you can use the GitHub web interface to create the commit for the test too. For that, you just need to navigate to the file while having the related fork branch to this PR selected. In this case, I think you should be able to edit using the GitHub web interface using this link: https://github.com/damonlynch/qtpy/edit/PySide6-QWebEngineScript-fix/qtpy/tests/test_qtwebenginewidgets.py

By the way, I was unable to understand how to set up pytest using the instructions in CONTRIBUTING.md. I ran the command pre-commit install, but I have no idea if that installed pytest.

pytest is installed when you run the command in this section: https://github.com/spyder-ide/qtpy/blob/master/CONTRIBUTING.md#install-qtpy-in-editable-mode

And to run pytest you can follow the info in this section: https://github.com/spyder-ide/qtpy/blob/master/CONTRIBUTING.md#running-the-tests

The pre-commit setup is meant to be added to run some code style rules when you commit changes so is not related with pytest but maybe the phrasing in that section can be confusing now that I'm rereading it 🤔 Thank you for the feedback on that!

If you have further questions, regarding how to add the changes to this PR, comments or want for us to help you finish this let us know!

@damonlynch
Copy link
Contributor Author

I didn't realise it was as easy as simply adding another commit. I have now done that, and pushed it to GitHub.

I also got confused by the section that explains how to install qtpy in editable mode, so I skipped that part. That means I have not personally run the changes I made to the test code in the most recent commit. However, given these changes are so simple, it probably doesn't matter in this instance.

Copy link
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 new commit @damonlynch ! I left some suggestions regarding the skip. Just in case, you can apply the suggestions in batch by going to the Files changed tab.

Let us know if you have further comments or questions!

qtpy/tests/test_qtwebenginewidgets.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtwebenginewidgets.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@damonlynch damonlynch left a comment

Choose a reason for hiding this comment

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

Code changes made as suggested, with a formatting change made by black

@dalthviz dalthviz changed the title PR: Fix bug when importing from PySide6.QtWebEngineCore (QWebEngineScrip vs QWebEngineScript) PR: Fix bug when importing from PySide6.QtWebEngineCore/QtWebEngineWidgets (QWebEngineScrip vs QWebEngineScript) Sep 7, 2023
Copy link
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 @damonlynch !

@dalthviz dalthviz merged commit 1faf764 into spyder-ide:master Sep 7, 2023
13 checks passed
@dalthviz dalthviz linked an issue Sep 8, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typo causes failed QWebEngineScript with PySide6
3 participants