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

Harmonization with OGC API Processes #27

Merged
merged 3 commits into from
Jun 16, 2021
Merged

Harmonization with OGC API Processes #27

merged 3 commits into from
Jun 16, 2021

Conversation

cportele
Copy link
Member

@cportele cportele commented Jun 9, 2021

This pull request closes #15 and closes #17.

@cportele cportele requested a review from jeffharrison June 9, 2021 12:39
@cportele
Copy link
Member Author

cportele commented Jun 9, 2021

I cannot add you as a reviewer @jerstlouis, so I mention you in this comment: please have a look.

@jerstlouis
Copy link
Member

jerstlouis commented Jun 11, 2021

Thanks @cportele
I really have been struggling going back & forth in my mind about the need for this extra "value".
Thinking that this will also be needed for a Maps extension allowing to specify custom inline styles object (profile of Processes, similar to Routes).

Then I read @fmigneault 's comments again and I think the most convincing arguments for the extra "value" (as opposed to trying to figure out a way to disembiguate pre-defined object schemas that can be substituted for it) is this comment, specifically:

Imagine JSON spec did the same, that they allowed you to write any schema, except you are not allowed to use a field named type because they use it internally to define their schema. Wouldn't it feel wrong?

The value adds that sub-object where we can safely tell the user: "do whatever you want in there, its your own schema".

and I think we took the right decision after all.

I had a look at the pull request and it looks good so far. After this is merged, I will have another go at updating the processes section in light of more recent changes to the process description (e.g. the switch from literalDataDomain/formats to JSON Schema for describing, and from id to key / values for, both inputs & outputs).

@cportele
Copy link
Member Author

Thanks @jerstlouis.

For Routes the "value" and "inputs" wrappers do not add any value, they just add noise. This seems to be different for Processes, because of the resource design in that standard.

I still have doubts that it is the right decision to add the wrappers in Routes, too, because in general Routes and Processes are separate resources and there is a trivial mapping/conversion for those that want to implement Routing using a Processes backend, but I accept that we add them in order to move forward with both Routes and Processes.

@jeffharrison
Copy link
Contributor

Please note - We will have this Issue as an Agenda Item for the SWG meeting.

@jeffharrison
Copy link
Contributor

16 June 2021 SWG Meeting discussed this PR. There were comments and discussions as to whether this addition adds too much 'noise' to the API. Key points are that it does not affect Route Exchange Model (REM) and it also does not 'break' the API in the opinion of SWG members. Members requested a specific Use Case where this is used. Response was to support a client that supports only Routes API and a 'generic' client that support Processes API. Question brought up... Can this be added later? Answer is No. This is a 'Do now or never' decision. Observation made that we are not developing a Processes profile. Adding this elements closes an 'enormous gap' between Routes resource approach and Processes resource approach. Attendees noted the cost is small with respect to adding this, and there may be benefits.

Caveat 1 - Would it help if a note was added to explain why we have these elements? Yes. Very important to include this before or as we merge.

The Use Case is to allow both generic Processes client and Routes client to function with API.

NOTUC to Merging this PR.

cportele added 2 commits June 16, 2021 21:33
... and the reasons for the additional members "inputs" and "value".

If there are proposals for improving the text, please prepare a PR.
@cportele cportele merged commit f0f0bb5 into master Jun 16, 2021
@cportele cportele deleted the processes branch June 16, 2021 19:54
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

Successfully merging this pull request may close these issues.

Can we make the RAPI request identical to a Processes Execute Request? 'Processes' Section Options
3 participants