Replacing pkg_resource with importlib.resources#474
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #474 +/- ##
==========================================
- Coverage 72.72% 72.66% -0.06%
==========================================
Files 18 18
Lines 1672 1672
==========================================
- Hits 1216 1215 -1
- Misses 456 457 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@danielhollas, can you please review these changes. I am not able to understand that why these tests are failing. |
pkg_resource with importlibpkg_resource with importlib.resources
danielhollas
left a comment
There was a problem hiding this comment.
@ayushjariyal great, tests are passing now!
Let's remove the last vestiges of pkg_resources in aiidalab/registry/web.py as part of this PR.
b1e24e9 to
ba42a15
Compare
|
@danielhollas , Done! |
danielhollas
left a comment
There was a problem hiding this comment.
Nice, thanks you!
I'll try to add a new test before merging this since the aiidalab.registry package is tested rather sparsely.
|
|
||
| def _walk_pkg_resources(package, root): | ||
| paths = pkg_resources.resource_listdir(package, root) | ||
| for path in paths: |
There was a problem hiding this comment.
Huh, it looks like you actually fixed a bug here where we would iterate over some directories twice before?
I'll verify a bit more, but the new implementation looks correct. I'll try to add a regression test for this.
There was a problem hiding this comment.
When writing a unit test for this, I would recommend printing paths here and comparing the old and new versions, they are not the same! (although the new one seems more correct as it doesn't duplicate the paths).
danielhollas
left a comment
There was a problem hiding this comment.
Hi @ayushjariyal,
apologies I did not get to this due to other work. If you'd like to push this forward, we'd need a bit more testing to ensure correctness of these changes, since the current testing of this code is very crude.
At the very least, we need unit tests for copy_static_tree_from_package and _walk_pkg_resources.
|
Hi @ayushjariyal. Do you still plan to finalize this one? Great if we can merge it soon 🙂 |
|
Hi @edan-bainglass, thanks for checking in! I'm still interested in getting this merged. I haven't been actively following the project recently, so I'm not very familiar with the latest changes in the codebase. If the PR looks good from your side and can be merged as is, that would be great. Otherwise, let me know what's needed and I'll take a look. Thanks! |
Thanks @ayushjariyal. You can refer to the last few comments in this PR for the remaining requirements. @danielhollas can provide further info if needed 🙂 |
Closes #432