Skip to content
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

Closed
wants to merge 13 commits into from
Closed

Conversation

m-mohr
Copy link
Member

@m-mohr m-mohr commented Feb 17, 2023

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...):
image

Has been tested successfully against the GEE back-end.

@m-mohr m-mohr requested a review from soxofaan February 17, 2023 15:17
@m-mohr m-mohr linked an issue Feb 17, 2023 that may be closed by this pull request
Copy link
Member

@soxofaan soxofaan left a 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

openeo/rest/userfile.py Outdated Show resolved Hide resolved
openeo/rest/userfile.py Outdated Show resolved Hide resolved
openeo/rest/userfile.py Outdated Show resolved Hide resolved
openeo/rest/userfile.py Outdated Show resolved Hide resolved
openeo/rest/userfile.py Show resolved Hide resolved
openeo/rest/connection.py Outdated Show resolved Hide resolved
openeo/rest/connection.py Outdated Show resolved Hide resolved
openeo/rest/userfile.py Show resolved Hide resolved
openeo/rest/userfile.py Outdated Show resolved Hide resolved
openeo/rest/userfile.py Outdated Show resolved Hide resolved
openeo/rest/connection.py Outdated Show resolved Hide resolved
openeo/rest/connection.py Outdated Show resolved Hide resolved
openeo/rest/connection.py Outdated Show resolved Hide resolved
openeo/rest/connection.py Outdated Show resolved Hide resolved
openeo/rest/connection.py Outdated Show resolved Hide resolved
openeo/rest/userfile.py Outdated Show resolved Hide resolved
openeo/rest/userfile.py Outdated Show resolved Hide resolved
openeo/rest/userfile.py Outdated Show resolved Hide resolved
openeo/internal/jupyter.py Outdated Show resolved Hide resolved
openeo/rest/userfile.py Show resolved Hide resolved
@m-mohr m-mohr marked this pull request as ready for review March 6, 2023 12:00
@m-mohr
Copy link
Member Author

m-mohr commented Mar 6, 2023

Updated according to the review, seems to work fine:

image

Ready for final review, I think.

@soxofaan
Copy link
Member

soxofaan commented Mar 6, 2023

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 Show resolved Hide resolved
"""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)
Copy link
Member

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

Suggested change
self.path = Path(path)
self.path = PurePosixPath(path)

because it is a non-concrete, posix-style path on the back-end

Copy link
Member

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.

Copy link
Member Author

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...

openeo/rest/userfile.py Outdated Show resolved Hide resolved
@m-mohr
Copy link
Member Author

m-mohr commented Mar 6, 2023

@soxofaan Fixed the remaining comments and applied the pre-commit stuff...

@soxofaan
Copy link
Member

soxofaan commented Mar 6, 2023

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.
@soxofaan
Copy link
Member

soxofaan commented Mar 6, 2023

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
soxofaan added a commit that referenced this pull request Mar 6, 2023
soxofaan added a commit that referenced this pull request Mar 6, 2023
Because upload creates a new UserFile it is more untuitive to have main logic in Connection.
soxofaan added a commit that referenced this pull request Mar 6, 2023
Finetune/fix UserFile docs as well
soxofaan added a commit that referenced this pull request Mar 6, 2023
@soxofaan
Copy link
Member

soxofaan commented Mar 6, 2023

I rebased this branch (to address wrongly merging of CHANGELOG.md) and merged in d16417c

thanks for pushing this!

@soxofaan soxofaan closed this Mar 6, 2023
@soxofaan soxofaan deleted the add-files-endpoints branch March 6, 2023 17:05
@m-mohr
Copy link
Member Author

m-mohr commented Mar 6, 2023

Thanks for completing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incomplete /files support?
2 participants