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

Consolidate REST error responses #8263

Closed
qstokkink opened this issue Nov 13, 2024 · 8 comments · Fixed by #8264
Closed

Consolidate REST error responses #8263

qstokkink opened this issue Nov 13, 2024 · 8 comments · Fixed by #8264
Assignees
Milestone

Comments

@qstokkink
Copy link
Contributor

qstokkink commented Nov 13, 2024

We inconsistently use REST error responses.

2 instances:

{
  error: {
    handled: boolean,
    message: string,
  }
}

3 instances:

{
  error: {
    handled: boolean,
    code: string,
    message: string,
  }
}

52 instances:

{
  error: string
}

This is bad because it causes error handling constructions like this:

if (response === undefined) {
setError(`${t("ToastErrorGetMetainfo")} ${t("ToastErrorGenNetworkErr")}`);
} else if ('error' in response && typeof response.error === 'object') {
let errorCode = response.error as {handled: boolean, code: string, message: string};
setError(`${t("ToastErrorGetMetainfo")} ${errorCode.code}`);
} else if (isErrorDict(response)) {
setError(`${t("ToastErrorGetMetainfo")} ${response.error}`);
} else if (response) {

@qstokkink qstokkink added this to the 8.1.0 milestone Nov 13, 2024
@qstokkink
Copy link
Contributor Author

@egbertbouman what do you think of cosolidating this to:

{
  error: {
    handled: boolean,
    message: string,
  }
}

For the errors with a code we would port the code field by including it in the message. For the errors that currently only have an error message we would put them in a {handled=True, message="previously the 'error' message"}.

@egbertbouman
Copy link
Member

Sounds good to me 👍

@qstokkink
Copy link
Contributor Author

Thanks 🙏 I'll give this a go then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants
@qstokkink @egbertbouman and others