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

Improve error messages on CLI HTTP request errors #5519

Merged
merged 2 commits into from
Dec 31, 2024

Conversation

xmbhasin
Copy link
Contributor

@xmbhasin xmbhasin commented Dec 31, 2024

Overview

What does this change accomplish and why?

This PR improves error messages when an HTTP request returns an unexpected status code, typically an error code.

Previously, the error message dumped the full request and response data without a friendly summary or message:

sincere-armadillo/main> pull unison/base

  Oops, I received an unexpected status code from the server.

  Here is the request.

    Request {..}
...

This PR surfaces the HTTP response status code and body message so a user can understand what went wrong and perhaps how to fix it at a glance:

scratch/main> pull unison/base

  I received an unexpected status code from the server: 400

  Response body: Unable to parse parameter name, Project shorthand must be of the form @user/project

  Request ID: 20300f6e-e0ac-4bc7-b882-bd127f8c0eb2

  Here is the request:

    Request {..}

...

If relevant, which Github issues does it close?

I didn't find an existing issue for this with a brief search.

Implementation notes

Updates Unison.CommandLine.OutputMessages to summarize HTTP response error info when a Servant.FailureResponse is encountered.

Interesting/controversial decisions

I reused the verbiage from code paths that handle Share.UnexpectedResponse in OutputMessages.

Initially, the use of "we" was kept from the original message used by Share.UnexpectedResponse.

As suggested, the message was changed to use I instead of We for consistency with other UCM CLI messages.

Test coverage

Have you included tests (which could be a transcript) for this change, or is it somehow covered by existing tests?

New automated tests are not included in this PR and error case tests for pull or similar CLI commands do not seem to be included in existing transcript tests.

I manually tested the following commands which are affected by this change:

  1. pull
  2. lib.install

A transcript test could be useful to protect against regression, but it requires integrating with an API server and may not be idempotent. When I tried adding a transcript test using the :error directive, the automatic error message also included a lot of ANSI escape code noise, making it difficult to interpret the test.

There may be other common commands that can hit a Servant.FailureResponse that should be tested.

Loose ends

@xmbhasin xmbhasin marked this pull request as ready for review December 31, 2024 02:29
@aryairani
Copy link
Contributor

Hi @xmbhasin, this is great.

Good catch on the controversial decision. It was an oversight that the "we" slipped in; and for UCM, we do want to stick with first-person, if you could swap it around?

@xmbhasin
Copy link
Contributor Author

@aryairani Thanks, I've updated the wording to use "I" instead of "we".

@aryairani aryairani merged commit b7b3439 into unisonweb:trunk Dec 31, 2024
17 checks passed
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.

2 participants