-
Notifications
You must be signed in to change notification settings - Fork 32
Improve reading error for selected plugins #377
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
ok these tests are unexpectedly failing cause they just passed locally. I think I did something weird. Please hold... |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #377 +/- ##
=======================================
Coverage 99.85% 99.85%
=======================================
Files 37 37
Lines 2711 2719 +8
=======================================
+ Hits 2707 2715 +8
Misses 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Ok this remaining error is totally not my fault, I will address in separate PR. Also this will probs break |
|
If you saw the original version, I was in the wrong env, naturally. |
| True, if the layer_data indicates an empty file, False otherwise | ||
| """ | ||
| return ( | ||
| isinstance(layer_data, list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it always list or can it be like a tuple?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think technically speaking it could be a tuple, but this function was reproduced from napari, so it's what we've always used to validate the return type of readers. I'd say we keep it like this for now, and change it later if need be. Looking at the docs for readers, we always refer to a list also.
When a plugin is selected by the user to read a file, we want to raise specific error messages when that reader fails to read, for whatever reason.
Prior to this PR:
get_readerfunction returnedNone, the error message would say that the pluginreturned no datanull_layer_sentinel, no error would be surfaced innapari. I think this is a holdover from olden days when we would cascade readers, so we didn't want thenull_layer_sentinelto throw an error. I think there's probably good reason to remove thenull_layer_sentinelentirely, but that's a discussion for another day.With this PR:
"Reader {plugin_name!r} was selected to open {paths!r}, but refused the file."null_layer_sentinel, we get"Reader {plugin_name!r} was selected to open {paths!r}, but returned no data."As an aside, if a plugin is not selected, we get a semi-generic
"No readers returned data"but I think this is also ok as, from thenapariside, it's ~impossible for npe2 reading to get called without a plugin selected - either we use the preferred one, the user specifically selects one, or we explicitly pass the only one available.Note that this brings the
_is_null_layer_sentinelfunction tonpe2, and it previously lived innapari. I think this is ok, and if we're happy with this PR, we can update its usage innaparito import from here.Note also that this function checks specifically for a
list(same as with napari/#7851), not a container sequence, but I actually think this is also ok and maybe we should update our typing in other places to simply expect a list, since many other container sequences wouldn't be useful anyway: adictwould likely fail somewhere weirdly, asetwould lead to unexpected layer orderings... I guess a generator would be ok? But I don't think anyone is returning anything except a list.