migrate from pkg_resources to importlib.resources
#38
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #38 +/- ##
==========================================
+ Coverage 67.31% 67.37% +0.05%
==========================================
Files 36 36
Lines 2772 2789 +17
==========================================
+ Hits 1866 1879 +13
- Misses 906 910 +4 ☔ View full report in Codecov by Sentry. |
|
Hi @paulmueller |
|
Hi @paulmueller |
paulmueller
left a comment
paulmueller
left a comment
There was a problem hiding this comment.
There are a few copy-paste errors.
Please test the application thoroughly (click on every menu item and open every possible dialog). The current tests do not cover the entire UI.
| pm = QtGui.QPixmap(impath) | ||
| ref = importlib.resources.files("pyjibe.img") / name | ||
| with importlib.resources.as_file(ref) as impath: | ||
| pm = QtGui.QPixmap(str(impath)) |
There was a problem hiding this comment.
This will not work. If impath exists, then pm is never created. PyCharm should give you a warning here as well.
There was a problem hiding this comment.
Yeah, I already fixed this. I will push it tomorrow.
|
Hi Paul |
| ref = importlib.resources.files("pyjibe.img") / name | ||
| with importlib.resources.as_file(ref) as ref_path: | ||
| impath = ref_path | ||
| pm = QtGui.QPixmap(str(impath)) |
There was a problem hiding this comment.
ref_path is guaranteed to exist only for the lifetime of the importlib.resources.as_file(ref) context. This means you must load the QPixmap inside the context
This is one of the main difficulties when migrating to importlib.resources.
One option would be to initialize pm with None and then, depending on whether the image path exists and pm is still None, override pm.
There was a problem hiding this comment.
Ok, that means if pm is None, I also have to raise an error. right?
There was a problem hiding this comment.
Correct, since the method is expected to return a QIcon
| if os.path.exists(impath): | ||
| pm = QtGui.QPixmap(str(impath)) | ||
| else: | ||
| ref = importlib.resources.files("pyjibe.img") / name | ||
| with importlib.resources.as_file(ref) as ref_path: | ||
| impath = ref_path | ||
| pm = QtGui.QPixmap(str(impath)) | ||
| pm = QtGui.QPixmap(str(ref_path)) |
There was a problem hiding this comment.
I wrote something like this. Since pyjibe.img.name exists in the repo, Do we need to initialize pm = None?
There was a problem hiding this comment.
While the directory pyjibe.img exists, you cannot be certain that an arbitrary file name exists in that directory.
Yes, we have to initialize pm=None and add a check in the end to make sure an image file has been found before using the pixmap.
There was a problem hiding this comment.
Ok, I added this line.
| with importlib.resources.as_file(ref) as ref_path: | ||
| pm = QtGui.QPixmap(str(ref_path)) | ||
| if pm is None or pm.isNull(): | ||
| raise ValueError(f"Loaded QPixmap is invalid for: {name}") |
There was a problem hiding this comment.
Please change the error message to: f"Could not find image '{name}'"
2 participants
This PR aims to implement migration from
pkg_resourcestoimportlib.resourcesas described in issue #33pkg_resourcescode withimportlib.resourcesand make necessary changes