Skip to content

#305: Add namespace to process graph nodes#309

Merged
m-mohr merged 3 commits intodraftfrom
namespaces
Jul 7, 2020
Merged

#305: Add namespace to process graph nodes#309
m-mohr merged 3 commits intodraftfrom
namespaces

Conversation

@m-mohr
Copy link
Member

@m-mohr m-mohr commented Jul 6, 2020

see #305 for reasons and discussions.

Please review carefully and approve, if this change is fine for you. Otherwise comment here.

@jdries
Copy link

jdries commented Jul 6, 2020

Does this include allowing URI's as process_id? Or will the namespace be a URI?

@m-mohr
Copy link
Member Author

m-mohr commented Jul 6, 2020

No URLs allowed yet. This seems to be too much of a change for 1.0, see also my #305 (comment). I think it might be better to leave URIs up to clients for now, but the API explicitly says this might be extended later. In this case it would be URI in namespace, because process_id at the moment only allows the \w+ pattern.

@soxofaan
Copy link
Member

soxofaan commented Jul 6, 2020

looks good, thanks for the quick PR

soxofaan added a commit to Open-EO/openeo-python-client that referenced this pull request Jul 6, 2020
@flahn
Copy link
Member

flahn commented Jul 6, 2020

just to understand this, the user will specify the namespace when sending an UDP to the back-end, right? If so, won't we need an enumeration for the namespaces that can be used on the back-end? Or ist it simply "user" and "predefined" or is it completely up to the user to chose any namespace?

@m-mohr
Copy link
Member Author

m-mohr commented Jul 6, 2020

@flahn Currently it's only "user" (= user-defined) and null (= pre-defined). There could be more in the future that a user could specify, but for now it's those two. Having that said, those two can easily be set by the clients and user's don't even need to notice the namespaces. When constructing a process graph with a builder, the client knows where the process originates from (you should request /processes anyway beforehand, probably also /process_graphs) and then can set the flag accordingly for the user.

@flahn
Copy link
Member

flahn commented Jul 6, 2020

Right. If it is limited to "user" and null this can be done without user interaction and can stay hidden. However, if it is planned in the future to use other namespaces I can't hide it. Then the user has somehow to be informed which namespaces they can specify and what it means.

@m-mohr
Copy link
Member Author

m-mohr commented Jul 6, 2020

Still, it would only be required to expose this for other namespace than the one listed above. If you read the issue, there are some examples what could be added in the future and yes, those would likely somehow need to be specified by the user, but even those could be somehow hidden that it's not too confusing. For example, the "URL" use-case could just be that a user specifies a URL and then the client can detect that it's a URL and set the namespace accordingly. But that's the future, for now it can be hidden in clients.

Copy link

@claxn claxn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine from my side! As soon as it is merged I will add this change/introduce this new label to the Python parser.

@m-mohr
Copy link
Member Author

m-mohr commented Jul 7, 2020

Seems to be consensus => merging.

@m-mohr m-mohr merged commit 843c6db into draft Jul 7, 2020
@m-mohr m-mohr deleted the namespaces branch July 7, 2020 17:20
m-mohr added a commit that referenced this pull request Jul 13, 2020
@m-mohr m-mohr mentioned this pull request Jul 13, 2020
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.

Processes: pre-defined + user-defined + sharing: namespacing/prefixing?

8 participants