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
FileSystem: handle encoding
via kwargs
.
#7694
Conversation
21e6bca
to
50a7f76
Compare
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.
Need to also update DVCFs and RepoFS
Drop if None. Closes #7691
encoding
via kwargs
.encoding
via kwargs
.
if "b" in mode: | ||
kwargs.pop("encoding", None) |
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.
Hm, why do we even need to pop it? In the #7691 we are not even getting the encoding here.
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.
RepoFS
and DvcFS
always sends encoding=None
to the filesystems even in binary mode. open()
is not fsspec compliant and can be fixed at that time in other filesystems. For now, I think this is the simplest approach.
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.
Let's fix repofs/dvcfs then if that is not too much work. Seems like it is a simple arg adjustment. Unless I'm missing something.
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'd still try to avoid this bug here even if we fix repofs/dvcfs, until we fix this on fsspec
's side.
Closes #7691