search

Open-EO / openeo-js-client

JavaScript and TypeScript client for the openEO API.
https://open-eo.github.io/openeo-js-client/latest/
Apache License 2.0
15 stars 6 forks source link

Improve parameters of `updateJob` etc. #14

Closed christophfriedrich closed 5 years ago

christophfriedrich commented 5 years ago

This issue talks about updateJob, but the same applies to updateService and updateProcessGraph too.

The original signature of updateJob was: updateJob(processGraph = null, outputFormat = null, outputParameters = null, title = null, description = null, plan = null, budget = null, additional = null)

This resulted in cases where one needed to type a lot of nulls to change e.g. the description: updateJob(null, null, null, null, 'new description')

To fix this, commit cc369798a2fbe83520741979e326af6ae3e9cd4a changed the signature to updateJob(parameters) so that one can simply pass an object: updateJob({description: 'new description'})

I generally like this, but also identified a few problems with it:

  1. Users must know which options they've got (increases need for/dependency on documentation)
  2. It's inconsistent with createJob where we still have all parameters instead of one object
  3. Jobs allow custom parameters, so we may run into problems with camelCase vs. snake_case
  4. As mentioned before, it's not consistent with the client dev guidelines (of openEO API v0.3.1)

Remarks on each:

  1. That is not really a problem, it's a common sight that libraries explain all the options in their docs
  2. We should unify this
  3. Need to keep that in mind, but I don't see big problems yet
  4. The openEO API v0.4 release's guidelines will emphasize more that developers can choose the option that works best in their language, and although we should try to adhere to the guidelines as much as possible it's still not binding.
m-mohr commented 5 years ago

Yes, there is room for improvements here. Thoughts:

  1. The options should be explained in the documentation of the client library, in addition I added a short remark in API 0.4.0: "The member variables / dictionary keys should use the same names as the parameters.".
  2. That's true, we should change that in the next version to something like createJob({...}, {budget => 123}).
  3. Yes, we probably need a proxy/decorator that automatically translates snake_case to camelCase and vice versa.
  4. It actually is consistent with the guidelines, see https://open-eo.github.io/openeo-api/v/0.3.1/guidelines-clients/#parameters
m-mohr commented 5 years ago

With API 0.4.0 and client version 0.4.0-beta.1 this was improved.

  1. The options are now explained in the docs.
  2. I don't think any longer that create and update are comparable. When you update you can only update a subset of properties, for example only the budget. In this case the other parameters shouldn't be specified as they should not change. When creating you can better work with the defaults that are specified. So I think the way it is now is okay.
  3. Standardized parameters are translated between snake_case and camelCase. We don't do this for proprietary parameters. I don't think that's a big deal we should write code for at the moment.
  4. See above.

I think this issue is solved for now.