Update SPE plugin for v3 API #963
Conversation
instead of os.path.getsize(). This should work with all file-like objects, even if they are not local files.
|
@FirefoxMetzger Could you have a look at this? I think the No-Internet Install check fails due to unrelated problems (it has worked before). |
|
Yes, the test fails, because it exceeds GH's rate limit for pulling images. I have seen it fail sporadically due to a network timeout (rare), but not yet due to rate limiting issues. I'll re-run it and it should dissapear. |
|
Thanks! Do you have suggestions concerning the failing codecov checks? Most of the complaints concern reading of metadata not present in the test file. All I can think of would be to patch the header of the test file using a hex editor to test the additional code paths. |
|
I will have to look into the code itself (the current plan is to do it this afternoon) before I can answer your question on coverage @ischr . If I can find a way to create a test file I will propose it, otherwise we can also continue without having the plugin covered 100%, since the old plugin wasn't fully covered either. I prefer full coverage though, so I want to make that option work if possible. |
lschr
force-pushed
the
spe_v3
branch
2 times, most recently
from
e4037b0 to
60bee6e
Compare
This is achieved by selectively patching the test file's header.
|
Sorry @lschr for the delay on reviewing this PR. I haven't forgotten, but want to have #824 merged before starting work on this. The work here replaces the SPE plugin with a v3 version, which is amazing 🤩 but will make SPE unavailable in v2 until we merge the aforementioned PR. Ideally, I would like this to be backwards compatible, hence the delay. |
FirefoxMetzger
left a comment
FirefoxMetzger
left a comment
There was a problem hiding this comment.
Fantastic work @lschr I really like what you did with the SPE plugin in this PR. Most of my comments are related to docstrings (added quite a few periods . in various places as well as some type indicators to the parameters).
There is only one bigger comment in imread where we could combine the case of reading index=... and index=int. Otherwise the PR is ready to roll :)
Also, sorry for taking so long to get around to reviewing this. Life has been somewhat of a roller coaster these past few weeks, but I'm happy to have found the time today to take a look at this. It's always fun to review clean and well-written code.
Co-authored-by: Sebastian Wallkötter <sebastian@wallkoetter.net>
FirefoxMetzger
left a comment
FirefoxMetzger
left a comment
There was a problem hiding this comment.
Nice work @lschr ! One final change in the docstrings and then we can merge 🚀
Co-authored-by: Sebastian Wallkötter <sebastian@wallkoetter.net>
|
LGTM! Awesome work @ischr ; thanks for upgrading our SPE plugin 🚀 |
2 participants
No description provided.