Use importlib instead of imp #168
Conversation
|
@syrusakbary would you mind taking a look and merging the PR? This issue is currently blocking my effort in one of the projects to move to Python 3.12 |
| def _load_source(module_name, filepath): | ||
| spec = importlib.util.spec_from_file_location(module_name, filepath) | ||
| module = importlib.util.module_from_spec(spec) | ||
| sys.modules[module_name] = module |
There was a problem hiding this comment.
Is it necessary to write to sys.modules for this to work?
There was a problem hiding this comment.
No, writing to sys.modules is not required for this to work (it just improves performance if the same python module is loaded multiple times)
There was a problem hiding this comment.
@paulmelnikow I'm not the owner of the PR, but I've found a similar problem and question on official Python mailing lists: https://discuss.python.org/t/how-do-i-migrate-from-imp/27885
It looks like writing to sys.modules is indeed correct, see this answer
EDIT: Didn't see the reply by @MarcellPerger1. My bad, sorry
There was a problem hiding this comment.
I guess my ask here is that it would be good to add some links to the imp documentation, mailing list, etc. to explain this implementation, which otherwise seems somewhat peculiar.
There was a problem hiding this comment.
@paulmelnikow I've made some comment/documentation suggestions to the PR as suggested in https://github.com/syrusakbary/snapshottest/pull/168/files#r1372331751
Let me know what you think of them.
Co-authored-by: Piotr Waszkiewicz <[email protected]>
There was a problem hiding this comment.
This looks good to me. Leaving open for a moment in case @MarcellPerger1 or anyone else would like to comment.
|
Any chance in merging this one? 🙏 |
|
Tested with both python3.10 and python3.12. Can confirm 3.10 is still working and 3.12 is now also working. Any change of merging? |
|
I used the following in our Pipenv file to reference this branch, to run our tests snapshottest = { git = "https://github.com/syrusakbary/snapshottest.git", ref ="refs/pull/168/head"} |
|
Hello 👋 ! Is there any chance this might merged soon? Would love to have the package ready for py 3.12. Much appreciated! |
|
Yeah, tripping over this as soon as I try to use python 3.12; approving it would be swell. |
|
@Waszker || @paulmelnikow || @syrusakbary , |
Sorry @RobinTail I gave my approval but I don't have write access to merge the changes. |
|
I just changed to use syrupy instead. Migration was easy as 🍰 |
What about Django support? I have one project that uses django test runner which seems not supported by syrup. I'd be happy to make a switch as well! |
Almost 9 since the PR date. |
|
And 3.5 years from the last 1.0.0a0 alpha release. Maybe it's time to talk about forking, if anyone is interested in undertaking the commitment of continued maintenance. |
I don't know, @RobinTail. But we use pytest, so if you're correct, I wouldn't have noticed any problem 😊 |
|
I opened another PR before I saw this one. @paulmelnikow @syrusakbary |
Adapted from syrusakbary/snapshottest#168
Use importlib instead of imp, fixes #166