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

unify error messages via templates #2697

Open
markus2330 opened this issue May 13, 2019 · 2 comments

Comments

Projects
None yet
3 participants
@markus2330
Copy link
Contributor

commented May 13, 2019

At the moment very similar error/warnings have irrelevant differences or are missing important information (e.g. #2660).

I propose that we get templates (macros with predefined text) for some of the error categories. E.g. for validation errors, in nearly all cases, the error message template could look like:

Sorry, the validation of the key '<key>' failed because the value '<value>' is not one of the allowed values '<allowed values>'.
E.g. for the range plugin:
Sorry, the validation of the key '/somewhere' failed because the value '5' is not one of the allowed values '7-9'.
In the code one would use:
ELEKTRA_SET_VALIDATION_ERROR_T(key, value, allowed_values)

Of course it would not work with every error message but it would be helpful in common cases.

@kodebach

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

It is a nice idea, but the use of this macro would be complicated. The allowed_values probably has to be constructed out of other parts. Without the macro you can use something like ELEKTRA_SET_VALIDATION_SEMANTIC_ERRORF(key, "[...] of the allowed values '%d-%d'", min, max). With the macro that would require additional malloc/free.

Also I think your template is to restrictive. Many validation plugins just have a condition that the value has to meet (reference to existing key, must be integer, match regex, etc.). Something like this may work better:

Sorry, the validation of key `<key>` with value '<value>' failed, because <reason>.

Where <reason> could be "it doesn't reference an existing key", "the value is not an integer" or "the value didn't match the regex [...]". But this still has the problem of complicated use in many cases.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

The allowed_values probably has to be constructed out of other parts.

As said: not in every situation it is best to use the macro. If you have exactly these arguments, they will help. If one argument is missing, you will hopefully rewrite the code so that you deliver the needed information. (Which is the main purpose of these templates: to give a guideline which information should be present.)

We already have such macros, e.g. ELEKTRA_MALLOC_ERROR and ELEKTRA_SET_ERROR_READ_ONLY, see src/include/kdbmacros.h
They were introduced to make the error handling more consistent.

My proposal here is to unify them and to match them with the categories we have.

Also I think your template is to restrictive. [...] , because

We decided to drop the separation of "description" and "reason". Furthermore, "reason" would be typically constructed out of other parts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.