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

add either extractor #1788

Merged
merged 3 commits into from
Nov 20, 2020
Merged

add either extractor #1788

merged 3 commits into from
Nov 20, 2020

Conversation

robjtede
Copy link
Member

@robjtede robjtede commented Nov 15, 2020

PR Type

Feature

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

Overview

Add extractor helper using the existing Either<A, B> type to handle fallbacks, like Either<Json<Login>, Form<Login>> for example.

closes #1724

@robjtede robjtede requested review from a team November 15, 2020 17:16
@robjtede robjtede marked this pull request as ready for review November 15, 2020 17:16
#[derive(Debug, PartialEq)]
pub enum Either<A, B> {
/// First branch of the type
A(A),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why A over Left if the docs and test code use left / right terminology rather than A / B?

Copy link
Member Author

@robjtede robjtede Nov 15, 2020

Choose a reason for hiding this comment

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

It's because this isnt a new type (i just moved the either parts out of responder.rs), it's in my head to rename the variants in next breaking release.

src/types/either.rs Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 15, 2020

Codecov Report

Merging #1788 (d9b83ca) into master (a929209) will increase coverage by 0.11%.
The diff coverage is 70.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1788      +/-   ##
==========================================
+ Coverage   53.79%   53.90%   +0.11%     
==========================================
  Files         129      130       +1     
  Lines       12201    12244      +43     
==========================================
+ Hits         6563     6600      +37     
- Misses       5638     5644       +6     
Impacted Files Coverage Δ
src/error.rs 89.18% <ø> (ø)
src/lib.rs 68.42% <ø> (ø)
src/responder.rs 87.84% <ø> (+4.59%) ⬆️
src/types/either.rs 70.14% <70.14%> (ø)
src/types/json.rs 89.85% <0.00%> (-0.97%) ⬇️
src/types/payload.rs 85.71% <0.00%> (-0.60%) ⬇️
src/service.rs 80.55% <0.00%> (-0.47%) ⬇️
src/request.rs 94.86% <0.00%> (-0.40%) ⬇️
src/middleware/logger.rs 77.34% <0.00%> (-0.33%) ⬇️
src/rmap.rs 94.84% <0.00%> (-0.03%) ⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a929209...d9b83ca. Read the comment docs.

@robjtede robjtede added C-feature Category: new functionality A-web project: actix-web B-semver-minor and removed C-feature Category: new functionality labels Nov 15, 2020
Copy link
Contributor

@fakeshadow fakeshadow left a comment

Choose a reason for hiding this comment

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

lgtm

@fakeshadow
Copy link
Contributor

fakeshadow commented Nov 16, 2020

I like this extractor. The only thing I would like to improve on is avoid using another box.
Maybe something like this https://gist.github.com/fakeshadow/7f42a2b82ab46f7ae417a6233764dba7.
It's not necessary or make things better by any means though.

@robjtede
Copy link
Member Author

We'll leave it for now. A box is very little considering the rest of this change.

One thing I'd just like an explicit thought on is the choice to return the error for A in the case of both failing. Currently the B error is never made available. The alternatives are to only show B's error or to return both errors in a tuple.

robjtede and others added 3 commits November 20, 2020 00:32
Co-authored-by: Jonas Platte <jplatte@users.noreply.github.com>
@robjtede robjtede merged commit 4100c50 into master Nov 20, 2020
@robjtede robjtede deleted the feat/either-extract branch November 20, 2020 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-web project: actix-web B-semver-minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implement FromRequest for Either<A, B>
3 participants