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

Client Guidelines vs ImagecollectionClient vs both #61

Closed
bgoesswe opened this issue Jul 1, 2019 · 4 comments
Closed

Client Guidelines vs ImagecollectionClient vs both #61

bgoesswe opened this issue Jul 1, 2019 · 4 comments

Comments

@bgoesswe
Copy link
Member

bgoesswe commented Jul 1, 2019

Hey, I wanted update the processes of the client to v0.4.1, but at the moment we define processes at two places in the code:

I feel like the first one is the one the python developer in this project like the most and the second one is the correct one according to the client guidelines.

So from my point of view we can keep both, but we should define the processes only at one place, so there are three possibilities I can think of:

  1. Define all processes in "ImagecollectionClient" (which is atm more up-to-date regarding existing processes) and use it for "Processes" also (internally).

  2. Define all processes in "Processes" (which is atm more up-to-date regarding to the guidelines, and therefore all possible parameters) and use it for "ImagecollectionClient" also (internally).

  3. Create a new Class, where the Processes are defined and used in both classes.

What do you think?

@jdries
Copy link
Collaborator

jdries commented Jul 2, 2019

I would say that in terms of following the client guidelines, it might make more sense to build a proof of concept for a python client that dynamically 'discovers' the available processes on the backend, and then uses reflection to build an API from that at runtime, including documentation if possible. This is also what is suggested in #40 . That way, we can support more backends.

If we want to continue implementing the processes in 2 different ways, we have to:

  • avoid code duplication, but that is an internal aspect
  • provide clear documentation to the user as to why this difference is there, and what he should use. Also each sample should take this into account, as it has already caused confusion in the past.

@soxofaan
Copy link
Member

The duplication between the two is indeed cumbersome (just noted that some things between the two are out of sync)

There is also another option:

  1. change the guidelines to allow the current python approach of image_collection.process_name(args) and drop process.py.

I think this "fluent" api style is pretty common in object oriented languages (the guidelines even recommend "method chaining").

Moreover, from my reading of the guidelines, it doesn't really feel that the image_collection.process_name(args) approach is violating them to be honest. If you consider a ImageCollection object to be a scope, you can say that the approach follows the guidelines. If there is agreement on this, I think there is little to be changed in the guidelines (at least let the Python workflow example of https://open-eo.github.io/openeo-api/v/0.4.1/guidelines-clients/#python-mixed-style reflect the current python client approach better)

@soxofaan
Copy link
Member

As a sidenote and related to the comment of @jdries above: I think it might be useful to add annotations/decorations to the "process" related methods in ImageCollectionClient for documentation purposes and to allow some auto-detection features (warn about processes that are not supported by backend, check methods and type hints against spec, ...). But that's probably better for another ticket

@jdries
Copy link
Collaborator

jdries commented Nov 29, 2019

Closing this one due to issue cleaning: for the near future, we plan to continue building the client with a focus on usability for Python users.

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

No branches or pull requests

3 participants