Skip to content

check for bytes return type and dictifiy it #710

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

Merged
merged 1 commit into from
Jul 15, 2021

Conversation

d-v-b
Copy link
Contributor

@d-v-b d-v-b commented Jul 15, 2021

Fixes #707

self.fs.cat returns an instance of bytes if keys2 has one element, which causes an error when fsmap.getitems attempts to access the items property of that value. This PR checks if self.fs.cat returned an instance of bytes, and if so, sticks it in a dict. This kind of problem might be an argument for type annotations, which would have flagged this issue earlier.

@d-v-b d-v-b mentioned this pull request Jul 15, 2021
@martindurant
Copy link
Member

might be an argument for type annotations

I am not against it in principle, but where I've been forced to use it myself (pandas, xarray), it took far more effort to twist the type system to what I need, and several errors in that process. Perhaps my coding style is inherently type-unfriendly.

@martindurant
Copy link
Member

I suppose it's worth a test?
NB: the s3fs failure is some intermittent conda problem.

@martindurant
Copy link
Member

Can someone confirm that this fixes xarray? @keewis

@keewis
Copy link
Contributor

keewis commented Jul 15, 2021

it does! I can confirm that the xarray test fails with 2021.07.0 and passes with this branch

@martindurant
Copy link
Member

Rerunning tests, will merge when green.

@martindurant martindurant merged commit 77c783e into fsspec:master Jul 15, 2021
@martindurant
Copy link
Member

Thank you @d-v-b . I still don't quite understand why this became an issue now, but never mind!

@jakirkham
Copy link

Would it be possible to tag a release with this change @martindurant ? 🙂 No worries if you are busy

@martindurant
Copy link
Member

Waiting on #731

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

return type of fs.cat
4 participants