-
Notifications
You must be signed in to change notification settings - Fork 40
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
Support for the /files endpoints #377 #378
Conversation
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 already good
a couple of quick notes
Co-authored-by: Stefaan Lippens <soxofaan@users.noreply.github.com>
982a5a9
to
c040bab
Compare
Co-authored-by: Stefaan Lippens <soxofaan@users.noreply.github.com>
side-note: we started using pre-commit for automated code cleanups in commits and PRs, you might want to enable that too. More info: https://open-eo.github.io/openeo-python-client/development.html#precommit |
openeo/rest/userfile.py
Outdated
"""Represents a file in the user-workspace of openeo.""" | ||
|
||
def __init__(self, path: str, connection: 'Connection', metadata: Dict[str, Any] = None): | ||
self.path = Path(path) |
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.
based on you usage example screenshot (which shows this as WindowsPath
), I now think this should be
self.path = Path(path) | |
self.path = PurePosixPath(path) |
because it is a non-concrete, posix-style path on the back-end
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'm not sure however how well this plays with path construction operations elsewehere in this PR.
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.
Seems to work. I've made it so that PurePosixPath is only used for the back-end paths, but the download() methods still returns a path depending on the OS as it points to a locale file. But maybe also do a sanity check...
@soxofaan Fixed the remaining comments and applied the pre-commit stuff... |
FYI Pushed a commit with some basic test coverage and additional finetuning |
Because upload creates a new UserFile it is more untuitive to have main logic in Connection.
Also changed location of upload logic: I think it makes more sense on Connection than on UserFile because upload creates a new UserFile. That's weird with UserFile method because it's not clear that it invalidates the original UserFile instance. I would even consider removing the UserFile.upload method |
Finetune/fix UserFile docs as well
Because upload creates a new UserFile it is more untuitive to have main logic in Connection.
Finetune/fix UserFile docs as well
I rebased this branch (to address wrongly merging of CHANGELOG.md) and merged in d16417c thanks for pushing this! |
Thanks for completing this. |
Implements #377
It seems the Python client only allowed to list files so far, which was not very useful. Added full support for the /files endpoints.
What do you think, @soxofaan ?
It's available in all clients except for Python so I think it should be added to push towards full openEO API support.
Interface looks like this (filenames/paths don't make any sense...):
Has been tested successfully against the GEE back-end.