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

Fixes PyOtherSideQtRCImporter for submodule imports #134

Merged
merged 2 commits into from
May 31, 2024

Conversation

timegrid
Copy link
Contributor

@timegrid timegrid commented May 2, 2024

It looks like the importer fix for python 3.12 in #131 does not cover imports of submodules. In moment we use importNames() like this:

addImportPath("qrc:/src")
importNames("backend.qml_bridge", ["BRIDGE"], () => { [...] });

This stopped working with the upgrade to python 3.12, #131 did not fix it. When used for a submodule, the path variable is set to the value of __path__ from the parent package according to the reference. By removing this path is None condition, it works again. Not sure though, if this MR will break other usecases though.

@apollo13: could you please test this against your setup?

@apollo13
Copy link
Contributor

apollo13 commented May 2, 2024

Puh will see about testing that, I will try over the weekend. Can you check what path is in your case? Maybe it would be "more" correct to restore the initial check:
https://github.com/thp/pyotherside/pull/131/files#diff-453f8c4ed0f10bda6f41ccfd2ab26d7bb45ecc92355ac956b53c75ff9eb1962aL25

So essentially:

if path is None or all(x.startswith('qrc:') for x in path):

@timegrid
Copy link
Contributor Author

timegrid commented May 2, 2024

In our application the path parameters for different cases look like this:

backend -> None
backend.qml_bridge -> ['qrc:/src/backend']
backend.models.model_item ->  ['qrc:/src/backend/models']
cffi._pycparser -> ['/media/data/code/moment/venv/lib/python3.12/site-packages/cffi']

So excluding non qrc paths makes sense. Restoring this former check works, too, so I'll adjust this PR. Thanks for the hint.

@apollo13
Copy link
Contributor

apollo13 commented May 2, 2024

Yeah, that makes sense! Thank for double checking.

@thp
Copy link
Owner

thp commented May 31, 2024

@apollo13 @timegrid Is this ready to go in?

@apollo13
Copy link
Contributor

apollo13 commented May 31, 2024 via email

@thp thp merged commit 302c111 into thp:master May 31, 2024
3 checks passed
@timegrid
Copy link
Contributor Author

I agree. Additionally some moment users tested a patched version for a while, and everything seems fine with the imports.

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.

3 participants