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

added PySide6 shiboken6 support - Pt 2 #393

Closed
wants to merge 13 commits into from
Closed

Conversation

zoshua
Copy link
Contributor

@zoshua zoshua commented Feb 20, 2024

Now passing new testing framework!

@CLAassistant
Copy link

CLAassistant commented Feb 20, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ masqu3rad3
✅ zoshua
❌ joshochoa


joshochoa seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Qt.py Outdated
"QtCore.Slot": "QtCore.Slot",
"QtCore.QAbstractProxyModel": "QtCore.QAbstractProxyModel",
"QtCore.QSortFilterProxyModel": "QtCore.QSortFilterProxyModel",
"QtCore.QItemSelection": "QtCore.QItemSelection",
Copy link
Owner

Choose a reason for hiding this comment

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

Some of these map back to themselves, can we remove these?

Copy link
Contributor Author

@zoshua zoshua Feb 20, 2024

Choose a reason for hiding this comment

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

Absolutely.

Is it fair to assume that anything mapping to itself is redundant?

I am noticing this mapping similarly exists in the _misplaced_members["PySide2"] dict as well

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, I think so, I can't think of why these would exist. I expect it has been overlooked. The tests should reveal whether they were necessary, but feel free to trim as necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did a pass on the whole script to remove redundant mappings.

Test passed 👍

@mottosso
Copy link
Owner

Great news, thanks. So I think the reason tests don't run here is because of that CLA. It looks like you are both @zoshua and @joshochoa except @joshochoa is not a GitHub user.

Would it be possible to squash your commits, such that they are all committed under the same user?

From there, what's missing is tests actually running for PySide6. Currently, we only test whether the changes pass for Qt 4 and 5. For Qt 6, I expect we'll need to update the Dockerfile to include PySide6.

for squashing
@zoshua
Copy link
Contributor Author

zoshua commented Feb 21, 2024

Spent all morning trying to figure out the best way to 'fix' the orphaned user situation but have not really come up with a straightforward/elegant solution just yet.

Unfortunately there is no clear easy squash on my end that I could find in Github WebApp or Desktop.

The current solve is looking like a serious rewriting of the commit history using something like git filter-repo or git filter-branch.

This is definitely new territory for me, if you/anyone reading this has creative fixes I am happy to implement, lmk.

@MHendricks
Copy link
Collaborator

MHendricks commented Feb 21, 2024

This piqued my interest, and re-affirmed why I avoid merge commits like the plague in feature branches, they make updating the history of a feature branch branch harder. The merging in of the newer commits in mottosso:master made simple squashing difficult.

I tried a few things but I had the most success with generating a patch file based on this. It will squash all commits into a single new commit on top of the latest commit in mottosso:master.

I recommend making these changes on your working copies master branch so you don't need to create a new pull request.

  1. Make a backup. I normally create a branch or tag called temp. When done I know to remove it. If something goes wrong, I use git reset --hard <...> to reset the broken branch back to the original value.
  2. Stash any un-committed changes you may have in your working copy.
  3. Get the commit hash of the latest mottosso:master commit. Currently 2c0f4dc596204670f4597b841821cff8a171c3a2, I'll refer to it as <qt_py_hash>.
  4. With your zoshua:master branch checked out generate a patch file with using the commit hash": git diff <qt_py_hash> HEAD -- > updates.dif. This shows the sum of all changes between your branch and Qt.py, ie what we see in the changes tab of this MR.
  5. Now we want to reset your working copies zoshua:master branch to the current state of the Qt.py master branch. git reset --hard <qt_py_hash> again using the qt_py_hash. At this point your branch is exactly the same as mottosso:master.
  6. If you want you can run the git apply --stat updates.diff and git apply --check updates.diff mentioned in the stack overflow article, but they are just checking if the patch will apply.
  7. I couldn't get the git am command to work due to a empty ident name, but that seems to be actually committing the changes, you can just run git apply updates.diff. This apples your code changes as working copy changes.
  8. Commit the changes as normal using the correct username.
  9. At this point your zoshua:master should show the changes you want to make as a single commit. You will need to force push the changes to update this Pull Request's code.
  10. Once you are happy with all of this you can remove your backup branch/tag in your working copy.

Warning:

As this involves rebasing and force pushing, I'll add the obligatory warning that you should (almost)never rewrite the history of a master branch other people are using. As zoshua:master is your fork and I'm guessing you are the only user of I'm not worried, but at this point if mottosso were to do stuff like this to mottosso:master it would annoy every other user was working on a fork of it. For future merge requests I would recommend making a feature branch not directly editing zoshua:master.

@zoshua
Copy link
Contributor Author

zoshua commented Feb 21, 2024

@MHendricks Thank you so much for this write up!

I totally goofed on not working from a feature branch in the first place.

I will run through the process asap and let you know if I hit any snags.

@mottosso
Copy link
Owner

Eek, complicated. Since we are only interested in 1 file, I would recommend:

  1. Make a new branch from master
  2. Copy your Qt.py file into it
  3. Push

Now you've "squashed" it and we can PR and merge that instead.

@zoshua
Copy link
Contributor Author

zoshua commented Feb 25, 2024

Closing this one out as we moved the submission to PR 394

Thanks to everyone for the contribution and guidance.

@zoshua zoshua closed this Feb 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants