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 imported modules logic if 'FORCE_QT_API' is empty #230

Merged
merged 1 commit into from
Aug 14, 2021

Conversation

hiaselhans
Copy link

As mentioned in #222 i stumbled across this.

before:
Bildschirmfoto vom 2021-02-08 12-44-08

after (expected behaviour):
Bildschirmfoto vom 2021-02-08 12-47-17

It seems obvious to me and i spare further explination.

@CAM-Gerlach CAM-Gerlach changed the title fix imported modules logic if 'FORCE_QT_API' is empty Fix imported modules logic if 'FORCE_QT_API' is empty Mar 19, 2021
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

This does seem like the intended behavior based on the comment, though I'm not sure why it wasn't caught in the original PR #194 . This will affect backward compatibility. however, but no way around that due to how it was implemented previously.

@JasonGilholme any feedback here?

@CAM-Gerlach

This comment has been minimized.

@CAM-Gerlach CAM-Gerlach added this to the v1.10.0 milestone Mar 19, 2021
@hiaselhans
Copy link
Author

thanks @CAM-Gerlach for taking care of this quite fundamental package.

How can i trigger the gh-actions-based checks?
Maybe i should add a test? :)

@hiaselhans
Copy link
Author

Hi @CAM-Gerlach

any progress on the maintainers issue?
Qtpy seems to be a core module used in quite a few projects, even in Spyder and being left without maintainer.

looking at #229 #233 #225 and others i feel like many other packages are waiting for qtpy to bump its version, drop qt4 and support pyside6

@UmbralReaper UmbralReaper mentioned this pull request May 3, 2021
@hiaselhans
Copy link
Author

@andfoy @ccordoba12

could you please pay a little attention on this base package too? Or delegate work to a new maintainer.
I see quite some community action here and every now and then i get notifications of devs encountering the same issue.

@hiaselhans
Copy link
Author

another month, another reminder @andfoy @ccordoba12 ;)

@dalthviz dalthviz changed the title Fix imported modules logic if 'FORCE_QT_API' is empty PR: Fix imported modules logic if 'FORCE_QT_API' is empty Aug 7, 2021
@dalthviz dalthviz linked an issue Aug 10, 2021 that may be closed by this pull request
@dalthviz
Copy link
Member

Hi @hiaselhans could you remove the unrelated commits from this PR? Or if is better for you, maybe we can help you with that? Let us know :)

@hiaselhans
Copy link
Author

Hi @dalthviz !

great to see some news here…
I think those came from rebasing. If i open a new pr from the same branch they shouldnt be visible, right…?

@dalthviz
Copy link
Member

Thank you for the fast response @hiaselhans ! I think that now these commits are in the branch so if you create a new PR that uses the same branch will lead still to seeing them. What you could try is to create a new clean branch (using as base an updated master branch version) and then cherry pick the commits here that are actually related to the import logic for FORCE_QT_API (which I think is commit 3f676d9) or try to remove the commits that are unrelated to the import logic for FORCE_QT_API. Let us know if you need any help :)

@hiaselhans
Copy link
Author

Ok i think i can do it later today :)

@hiaselhans hiaselhans force-pushed the fix_force_qt_api branch 2 times, most recently from d724840 to 3f676d9 Compare August 13, 2021 07:22
@hiaselhans
Copy link
Author

hi @dalthviz

i removed all commits but that one :)
hope it is fine like that. Good to see some progress here!

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.

Thanks @hiaselhans ! LGTM 👍

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.

Imported modules are not respected
3 participants