-
Notifications
You must be signed in to change notification settings - Fork 368
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
Extend JSON serialization capabilities #1612
Conversation
At a first quick glance, this PR looks great. Can I ask what kind of workflow you have in mind to make use of this feature? |
I'm trying to set up document nodes that reference files in |
Sounds like something Intake could do for you, but I might be slightly biased :) |
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.
Very minor comments
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.
The whole encoder/decoder classes are quite long now, there's no reason they need to be in the main spec
module - can they be pulled into a different file?
fsspec/spec.py
Outdated
if (obj_cls := self.try_resolve_fs_cls(dct)) is not None: | ||
return AbstractFileSystem.from_dict(dct) | ||
if (obj_cls := self.try_resolve_path_cls(dct)) is not None: | ||
return obj_cls(dct["str"]) |
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.
If these both fail, you end up with just a dictionary?
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.
Yes, that is the default behaviour when decoding a dictionary from JSON.
fsspec/spec.py
Outdated
try: | ||
path_cls = _import_class(fqp) | ||
except Exception: | ||
raise |
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.
why catch Exception only to raise it again, all within the outer suppress block?
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.
It's just to have the same control flow as the code for decoding the filesystem class. If you think it's too verbose then I can simplify it.
Sure - I'll move them to |
I have finished addressing your comments. |
Co-authored-by: Martin Durant <[email protected]>
#247 first introduced JSON serialization of filesystem objects. However, it fails to handle
storage_args
andstorage_options
that containPath
or other filesystems. This can occur in a few cases, such as:Path
instance to construct aDirFileSystem
(not officially documented but thePath
object is automatically converted to a string in the initializer, so it is functional)CachingFileSystem
,DirFileSystem
andReferenceFileSystem
.This PR makes it possible to serialize such filesystems.