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
DM-27435: Unification of file-like datastores #426
Conversation
f855017
to
22d5443
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.
Looks good. I think all of my comments are on things you're already aware could use some cleanup, and none of them are critical for this ticket.
# There will be problems with the bytes we are supplying so ignore | ||
pass | ||
return True | ||
|
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.
Is the long-term plan here to implement this in subclasses directly, and then make it abstract?
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. I haven't worked out the best approach yet but there is a Formatter cleanup ticket. I wanted an API here though so that I could at least ask the formatter about it even if the implementation was a bit odd. I had hoped to get away with always using the presence of a method to declare it but it seems like I'm going to have to use the approach of a class property and implementation method.
with uri.as_local() as local_uri: | ||
# Have to update the Location associated with the formatter | ||
# because formatter.read does not allow an override. | ||
# This could be improved. |
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.
FWIW, this is indeed something I'd love to see fixed (but not on this ticket).
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 FileDescriptor class was one of the first classes that Pim wrote and I'm not really convinced I need it. Again I can deal with that on the formatter cleanup.
7e9a9f1
to
5fc9708
Compare
They are now deprecated and replaced by RemoteFileDatastore
This lets a URI from a temporary file be marked as such so that users can make decisions (for example when deciding whether certain transfer modes make sense).
This will save people from doing a check on the URI scheme
This allows temporary files to be deleted automatically.
The file reader had this logic but the bytes handler did not.
The implementation is not perfect since it relies on a method raising NotImplementedError since at the moment there is no class property to simply declare bytes support.
* PosixDatastore and RemoteFileDatastore now issued deprecation warnings.
Now has a context manager rather than directly changing the internal property of the formatter.
Nothing ever used it directly so it can be safely removed without a deprecation period.
bd5812e
to
19300f8
Compare
No description provided.