Skip to content

check for bytes return type and dictifiy it#710

Merged
martindurant merged 1 commit intofsspec:masterfrom
d-v-b:getitems_fix
Jul 15, 2021
Merged

check for bytes return type and dictifiy it#710
martindurant merged 1 commit intofsspec:masterfrom
d-v-b:getitems_fix

Conversation

@d-v-b
Copy link
Contributor

@d-v-b d-v-b commented Jul 15, 2021 •
edited by martindurant
Loading

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
Closed
@martindurant
Copy link
Member

martindurant commented Jul 15, 2021

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

martindurant commented Jul 15, 2021

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

@martindurant
Copy link
Member

martindurant commented Jul 15, 2021

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

martindurant commented Jul 15, 2021

Rerunning tests, will merge when green.

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

martindurant commented Jul 15, 2021

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

@kmsquire kmsquire mentioned this pull request Jul 17, 2021
Open
4 tasks
This was referenced Aug 20, 2021
Merged
Merged
@jakirkham
Copy link

jakirkham commented Aug 30, 2021

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

@martindurant
Copy link
Member

martindurant commented Aug 30, 2021

Waiting on #731

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

return type of fs.cat

4 participants

@d-v-b @martindurant @keewis @jakirkham