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

Add "namespace" placeholder to ProcessUnsupported error message (#305) #328

Merged
merged 3 commits into from
Nov 25, 2020

Conversation

soxofaan
Copy link
Member

No description provided.

@m-mohr
Copy link
Member

m-mohr commented Jul 22, 2020

Hmm, good point, but a breaking change again.

I need to think about how to manage the API from now on with patch, minor and major releases and changes.

@m-mohr m-mohr added the breaking Breaking changes, requires a major-version (2.0.0 for example) label Jul 22, 2020
@soxofaan
Copy link
Member Author

Is the message field of the errors part of the binding contract of the API? Aren't backends somewhat free to change the error message a bit anyway (e.g. removing or adding placeholders and information)?

e.g. https://openeo.org/documentation/1.0/developers/api/errors.html explicitly states

The following table of error codes is incomplete. These error codes will evolve over time.

@m-mohr
Copy link
Member

m-mohr commented Jul 22, 2020

Is the message field of the errors part of the binding contract of the API?

Well, at least my back-end imports the file directly and then it specifies just the placeholders in code, but if we add new placeholders, then the error message will be incomplete (e.g. "Process with identifier 'ndvi' is not available in namespace '{namespace}'."). So in that sense it's breaking and unexpected.

Aren't backends somewhat free to change the error message a bit anyway

Yes, sure. But that's not breaking others error messages. ;-)

e.g. https://openeo.org/documentation/1.0/developers/api/errors.html explicitly states

The following table of error codes is incomplete. These error codes will evolve over time.

Yes, but that's more meant to say that we'll add more error codes and not that we change the existing ones. (And in addition that message is still from the pre-stable era.) The official description is here: https://github.com/Open-EO/openeo-api#repository

@m-mohr
Copy link
Member

m-mohr commented Jul 22, 2020

Still this feels like a bugfix, so I could probably be convinced to include errors.json changes in a minor release (1.1). 🤔

@soxofaan
Copy link
Member Author

soxofaan commented Jul 23, 2020

Calling a minor error message change a bugfix sounds a bit like a stretch to me, especially because the messages are just suggestions for the backend implementer. In the VITO backend we also use the errors.json file (to generate exception classes in a build-time fashion), but I would call this more an implementation detail than a matter of following/violating the API.

That said, because it's just a suggestion, I'm fine with only merging in a minor release

@m-mohr
Copy link
Member

m-mohr commented Nov 25, 2020

As we'll likely introducing somewhat breaking changes in 1.0.1 for CORS, I'll merge this now for the 1.0.1 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking changes, requires a major-version (2.0.0 for example)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants