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

remove json and url encoded form support from -http #2148

Merged
merged 3 commits into from Apr 12, 2021
Merged

Conversation

robjtede
Copy link
Member

@robjtede robjtede commented Apr 9, 2021

PR Type

Dep Reduction

PR Checklist

  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • A changelog entry has been made for the appropriate packages.
  • Format code with the latest stable rustfmt.
  • (Team) Label with affected crates and semver status.

Overview

Further reduce the number of dependencies on -http, making it simpler.

Removes some Into<Error> and ResponseError impls for each lib's error types. I think this makes sense anyway but need to look at the error system before v4 anyhow, it's possible the response error trait can be moved to -web, too, which would make it possible to re-impl for these types.

@robjtede robjtede added B-semver-major breaking change requiring a major version bump A-http project: actix-http labels Apr 9, 2021
Base automatically changed from no-cookies-for-you to master April 9, 2021 17:07
@robjtede robjtede marked this pull request as ready for review April 10, 2021 18:03
@@ -209,7 +209,8 @@ impl RequestSender {
) -> SendClientRequest {
let body = match serde_json::to_string(value) {
Ok(body) => body,
Err(e) => return Error::from(e).into(),
// TODO: own error type
Err(e) => return Error::from(io::Error::new(io::ErrorKind::Other, e)).into(),
Copy link
Member Author

Choose a reason for hiding this comment

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

Thoughts on what do do here @fakeshadow, maybe a wrapped error that can also hold serialization errors as well as client errors from -http. Might leave it for a separate PR if you want to handle it.

@robjtede robjtede requested review from a team April 10, 2021 19:23
Copy link
Member

@popzxc popzxc left a comment

Choose a reason for hiding this comment

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

In general LGTM.

actix-http/src/response.rs Show resolved Hide resolved
@robjtede robjtede merged commit 981c544 into master Apr 12, 2021
@robjtede robjtede deleted the remove-json branch April 12, 2021 09:30
@tjkirch
Copy link

tjkirch commented Apr 30, 2021

Howdy! Bit of feedback on this, particularly the error changes, since it wasn't clear to me how to operate without those helpers. Up through beta.5, I was able to do things like HttpResponse::BadRequest().body(err.to_string()).into() in an error_handler, but now there would be more manual conversion required, since the Into is gone. I hope that if the error changes stay in place for v4 final, there can be some wording in the docs/changelog about how to adapt existing code. (I had to update from v3 to v4 beta to be able to update tokio.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-http project: actix-http B-semver-major breaking change requiring a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants