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

Proposal: Add additional methods to RequestResult #28

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

zdimension
Copy link
Contributor

This PR adds the following methods to RequestResult:

  • deserialize_body: Deserializes the body without performing checks on status code or content.
  • ensure_status_ignore_body: Checks the status code while ignoring body.
  • expect_status_ignore_body: Checks the status code while ignoring body, panic on failure.
  • ensure_empty_body: Checks that the body is empty.
  • ensure_status_empty_body: Checks the status code and the emptiness of the body.
  • expect_status_empty_body: Checks the status code and the emptiness of the body, panic on failure.

The last two items, specifically, are an attempt to fix #25 while both

  • keeping the public-facing API unchanged for the sole existing use case (check status code & body)
  • providing both try (ensure_* -> Result) and panic (expect_* -> Either<(), !>) versions of the corresponding methods

Any criticism on the API structure in itself would be much appreciated.

  • is preservation of the public API surface important enough at this point in the project to justify opting for ensure_thing1_thing2 rather than a builder pattern (.ensure_thing1().ensure_thing2())? The latter, if chosen, would require breaking the current API since expect_status should then only check for status, not body as currently is the case.

- deserialize_body: Deserializes the body without performing checks on status code or content.
- ensure_status_ignore_body: Checks the status code while ignoring body.
- expect_status_ignore_body: Checks the status code while ignoring body, panic on failure.
- ensure_empty_body: Checks that the body is empty.
- ensure_status_empty_body: Checks the status code and the emptiness of the body.
- expect_status_empty_body: Checks the status code and the emptiness of the body, panic on failure.
@zdimension zdimension added the enhancement New feature or request label Jul 3, 2022
@@ -180,15 +180,15 @@ pub async fn delete_user() {
CONTEXT
.run(&request)
Copy link
Member

Choose a reason for hiding this comment

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

could we try to use each new request at least once (so we can cover them a bit on "real life" tests)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

use each new request at least once

wdym? the request here (l. 178)

let request = Request::delete(path!["users", id]);

is used twice (l. 181 & l. 189)

Copy link
Member

@cbonaudo cbonaudo Jul 4, 2022

Choose a reason for hiding this comment

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

I said request, I meant methods sorry (like deserialize_body, ensure_status_ignore_body, ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohh yes, of course, I'll add test cases for those!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -304,13 +304,53 @@ impl RequestResult {
///
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate this api because it allows us to use a basic command (ensure_status) to perform the meat of most of our needs, and have more specific commands.
Maybe it can mean that ensure_status could be renamed to reflect the 2 things it does, maybe it's not needed, not sure about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle empty response bodies
2 participants