[FIX] fs_storage: Add a KeyError Exception#457
Conversation
etobella
force-pushed
the
16.0-add-keyError
branch
from
deb6c92 to
6ae989e
Compare
etobella
force-pushed
the
16.0-add-keyError
branch
from
6ae989e to
ad8f2b3
Compare
|
Well, I will use this PR in my internal implementations for sure with gitaggregate... once the library is fixed we will not need it anymore probably. Enforcing the library might work locally, but might give problems in other systems (it implies that everyone checks the dependencies on their projects and force the right one) |
|
I raised #455 but actually think, that the solution proposed here is more general. |
If we want a more general solution we should catch all 'Exception' instances to avoid to add another kind of exception in the future for not yet known error... |
lmignon
left a comment
lmignon
left a comment
There was a problem hiding this comment.
Catching the ImportError is not the right approach since it assume that the implementation will always raise this kind of exception when it's not possible to load a specific protocol. Any exception occurring at this stage has the same consequence.... the protocol can't be loaded
This is necessary in order to avoid issues in case fsspec added a new protocol and forget the error key
etobella
force-pushed
the
16.0-add-keyError
branch
from
ad8f2b3 to
dc07e5b
Compare
|
/ocabot merge patch |
|
On my way to merge this fine PR! |
|
Congratulations, your PR was merged at c076c4b. Thanks a lot for contributing to OCA. ❤️ |
6 participants
This is necessary in order to avoid issues in case fsspec added a new protocol and forget the error key
Without this change, edi-framework and storage are failing (and other tests might happen too).
The real error is here:
https://github.com/fsspec/filesystem_spec/blame/50c5a2e161d85413e19e9e10d42de8b3cf56a921/fsspec/registry.py#L75-L77
An err key should be added, but it was forgotten. I created a PR in fsspec in order to fix it, but I think we can add this in order to avoid other errors.
fsspec/filesystem_spec#1804
@lmignon @simahawk