👌 IMPROVE: Replace deprecated imp with importlib #4848
Conversation
|
thanks @DirectriX01, note your link in the comment above is pointing to the wrong thing |
There was a problem hiding this comment.
thanks for the PR @DirectriX01 !
The link under "importlib documentation" currently links to the AiiDA source code, I guess you meant to insert a different link here?
A reference on how to adequately replace the load_package and load_source functions would indeed be useful, perhaps also for the commit message.
I guess you've looked at the imp source code and taken what seemed necessary?
Anyhow, the tests seem to pass...
Hm... the failing django test does look like it might have to do with this change. |
|
I'm really sorry for overlooking and copying the wrong link. Here is the Importlib documentation |
| # I could use load_module but it takes lots of arguments, | ||
| # then I use load_source | ||
| app_module = imp.load_source(f'rst{name}', full_path) | ||
| spec = importlib.util.spec_from_file_location(f'rst{name}', full_path) |
There was a problem hiding this comment.
typo?
| spec = importlib.util.spec_from_file_location(f'rst{name}', full_path) | |
| spec = importlib.util.spec_from_file_location(f'{name}', full_path) |
| app_module = imp.load_source(f'rst{name}', full_path) | ||
| spec = importlib.util.spec_from_file_location(f'rst{name}', full_path) | ||
| app_module = importlib.util.module_from_spec(spec) | ||
| sys.modules[f'rst{name}'] = app_module |
There was a problem hiding this comment.
same here (or perhaps I don't understand what this does).
is this some weird autocomplete of "firstname" in the editor?
There was a problem hiding this comment.
Yes, and I have changed them accordingly
|
@DirectriX01 By the way, I believe the tests should also run on your fork after you push, i.e. this is an easy way to check whether all tests pass before making a pull request (no worries about this one). |
Codecov Report
@@ Coverage Diff @@
## develop #4848 +/- ##
===========================================
+ Coverage 80.02% 80.03% +0.01%
===========================================
Files 515 515
Lines 36557 36574 +17
===========================================
+ Hits 29252 29269 +17
Misses 7305 7305
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
The previous errors were because it couldn't load a package from the |
There was a problem hiding this comment.
thanks for fixing @DirectriX01 , just some minor comments then good to go from my side
@chrisjsewell you agree?
I'm surprised that importlib provides no easier non-deprecated path for this - this is going to cause headaches for a lot of python packages when the functionality is removed...
This made the issue quite a bit more complicated than expected, so in retrospect it was not really a "good first issue" - apologies.
Co-authored-by: Leopold Talirz <leopold.talirz@gmail.com>
Co-authored-by: Leopold Talirz <leopold.talirz@gmail.com>
Co-authored-by: Leopold Talirz <leopold.talirz@gmail.com>
|
Hey Abhinav, thanks for taking on this issue and bringing the PR to completion (I consider the work done; just waiting for approval from @chrisjsewell ). I was wondering: are you still planning to submit a GSOC proposal? As you may be aware, the deadline is in ~3 hours, i.e. it's still possible but a bit tight ;-) |
chrisjsewell
changed the title
I've replaced it according to the official Importlib documentation