-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Ensure encoding["source"] is available for a pathlib.Path object #6974
Ensure encoding["source"] is available for a pathlib.Path object #6974
Conversation
Resolves Issue pydata#5888. With this change, the "source" encoding will be stored whether the filename is a string or a Path object.
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 it's also possible to pass the whole dataset as bytes & we don't want this to end off in ds.encoding["source"]
I hope my suggestion would take care of this.
Co-authored-by: Mathias Hauser <[email protected]>
Thanks. I had forgotten you could, for example, pass file-like objects as I don't tend to. I've confirmed your suggestion works and have added it to the change. |
Let's see if that works now - should auto-merge on green. Thanks for your contribution and welcome to xarray! |
It would be great to further improve this by handling file-like objects. I am running into an issue when trying to read some data from S3 buckets. I can pass in a file-like object and read the data, but there is no Can simulate this 'issue' using local data:
This will have no source encoding in the dataset, but it could be grabbed from Could a check be added for file-like objects, and then insert the |
Closes #5888. At the moment if you pass a
Path
object, thesource
will not be encoded in, only if a string is passed in. Given there is already a function to handle this,_normalize_path
, I've simplified the logic to just run that over thestr/Path
to ensure it is always encoded.whats-new.rst
I'm not sure if this needs including in the
whats-new.rst
. I'm happy to add it if desired.