-
Notifications
You must be signed in to change notification settings - Fork 36
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
Changes from 9 commits
638e33e
fd6b045
3d32669
271e063
4936c3a
f1e2849
c97e42c
1192a2c
4afcb4b
9da7d22
3ba1e3f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| from collections.abc import Sequence | ||
| from typing import ( | ||
| TYPE_CHECKING, | ||
| Any, | ||
| Literal, | ||
| cast, | ||
| overload, | ||
|
|
@@ -157,20 +158,32 @@ def _read( | |
| "No readers to try. Expected an exception before this point." | ||
| ) | ||
|
|
||
| tried_reader = False | ||
| for rdr in chosen_compatible_readers: | ||
| read_func = rdr.exec( | ||
| kwargs={"path": paths, "stack": stack, "_registry": _pm.commands} | ||
| ) | ||
| if read_func is not None: | ||
| tried_reader = True | ||
| # if the reader function raises an exception here, we don't try to catch it | ||
| if layer_data := read_func(paths, stack=stack): | ||
| return (layer_data, rdr) if return_reader else layer_data | ||
| layer_data = read_func(paths, stack=stack) | ||
| if plugin_name and _is_null_layer_sentinel(layer_data): | ||
| # we don't return null layers if the user selected a plugin, | ||
| # so that we can raise a meaningful error | ||
| continue | ||
| return (layer_data, rdr) if return_reader else layer_data | ||
|
|
||
| if plugin_name: | ||
| raise ValueError( | ||
| f"Reader {plugin_name!r} was selected to open " | ||
| + f"{paths!r}, but returned no data." | ||
| ) | ||
| if tried_reader: | ||
| raise ValueError( | ||
| f"Reader {plugin_name!r} was selected to open " | ||
| + f"{paths!r}, but returned no data." | ||
| ) | ||
| else: | ||
| raise ValueError( | ||
| f"Reader {plugin_name!r} was selected to open " | ||
| + f"{paths!r}, but refused the file." | ||
| ) | ||
| raise ValueError(f"No readers returned data for {paths!r}") | ||
|
|
||
|
|
||
|
|
@@ -256,6 +269,29 @@ def _get_compatible_readers_by_choice( | |
| return chosen_compatible_readers | ||
|
|
||
|
|
||
| def _is_null_layer_sentinel(layer_data: Any) -> bool: | ||
| """Checks if the layer data returned from a reader function indicates an | ||
| empty file. The sentinel value used for this is ``[(None,)]``. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| layer_data : LayerData | ||
| The layer data returned from a reader function to check | ||
|
|
||
| Returns | ||
| ------- | ||
| bool | ||
| True, if the layer_data indicates an empty file, False otherwise | ||
| """ | ||
| return ( | ||
| isinstance(layer_data, list) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it always list or can it be like a tuple?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I think technically speaking it could be a |
||
| and len(layer_data) == 1 | ||
| and isinstance(layer_data[0], tuple) | ||
| and len(layer_data[0]) == 1 | ||
| and layer_data[0][0] is None | ||
| ) | ||
|
|
||
|
|
||
| @overload | ||
| def _write( | ||
| path: str, | ||
|
|
||
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.
Should we still check if layer_data isn't None/False before this return?
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.
I'm pushing this change, and adding a test to see if that is the behavior we want. I think that what you say makes sense
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.
I think this change is ok. Technically the reader should never be returning falsy things, but many probably do, so I think this is more defensive.