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

+htc #20273 EitherUnmarshaller #20274

Merged
merged 1 commit into from Apr 22, 2016

Conversation

Projects
None yet
7 participants
@ktoso
Member

ktoso commented Apr 10, 2016

Resolves #20273 if we want to.
Seems like a nice addition to me but would welcome comments from @jrudolph @sirthias or @akka/akka-team

Related blog post http://kto.so/2016/04/10/hakk-the-planet-implementing-akka-http-marshallers/

@akka-ci akka-ci added validating tested and removed validating labels Apr 10, 2016

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Apr 10, 2016

Collaborator

Test PASSed.

Collaborator

akka-ci commented Apr 10, 2016

Test PASSed.

@johanandren

This comment has been minimized.

Show comment
Hide comment
@johanandren

johanandren Apr 11, 2016

Member

I can see why you choose the right hand to be the starting point but it feels backwards to me that
Unmarshal(...).to[Either[String, Int]] does not try String first.

Member

johanandren commented Apr 11, 2016

I can see why you choose the right hand to be the starting point but it feels backwards to me that
Unmarshal(...).to[Either[String, Int]] does not try String first.

@viktorklang

This comment has been minimized.

Show comment
Hide comment
@viktorklang

viktorklang Apr 11, 2016

Member

Why? Doesn't it make sense to pick the Right thing first, then take what's
Left? ;)

On Mon, Apr 11, 2016 at 6:11 PM, Johan Andrén notifications@github.com
wrote:

I can see why you choose the right hand to be the starting point but it
feels backwards to me that
Unmarshal(...).to[Either[String, Int]] does not try String first.


You are receiving this because you are on a team that was mentioned.
Reply to this email directly or view it on GitHub
#20274 (comment)

Cheers,

Member

viktorklang commented Apr 11, 2016

Why? Doesn't it make sense to pick the Right thing first, then take what's
Left? ;)

On Mon, Apr 11, 2016 at 6:11 PM, Johan Andrén notifications@github.com
wrote:

I can see why you choose the right hand to be the starting point but it
feels backwards to me that
Unmarshal(...).to[Either[String, Int]] does not try String first.


You are receiving this because you are on a team that was mentioned.
Reply to this email directly or view it on GitHub
#20274 (comment)

Cheers,

@johanandren

This comment has been minimized.

Show comment
Hide comment
@johanandren

johanandren Apr 11, 2016

Member

Sometimes the right thing is not the right thing.

Member

johanandren commented Apr 11, 2016

Sometimes the right thing is not the right thing.

@viktorklang

This comment has been minimized.

Show comment
Hide comment
@viktorklang

viktorklang Apr 11, 2016

Member

Make the right thing great again.

On Mon, Apr 11, 2016 at 6:14 PM, Johan Andrén notifications@github.com
wrote:

Sometimes the right thing is not the right thing.


You are receiving this because you are on a team that was mentioned.
Reply to this email directly or view it on GitHub
#20274 (comment)

Cheers,

Member

viktorklang commented Apr 11, 2016

Make the right thing great again.

On Mon, Apr 11, 2016 at 6:14 PM, Johan Andrén notifications@github.com
wrote:

Sometimes the right thing is not the right thing.


You are receiving this because you are on a team that was mentioned.
Reply to this email directly or view it on GitHub
#20274 (comment)

Cheers,

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Apr 11, 2016

Member

I always drive on the right side of the road, it's the english that do it on the wrong side.

Member

ktoso commented Apr 11, 2016

I always drive on the right side of the road, it's the english that do it on the wrong side.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Apr 11, 2016

Member

Hilarious puns aside, WDYT – should we stray away from the "left is error-ish" and evaluate Left-then-Right?

Member

ktoso commented Apr 11, 2016

Hilarious puns aside, WDYT – should we stray away from the "left is error-ish" and evaluate Left-then-Right?

@johanandren

This comment has been minimized.

Show comment
Hide comment
@johanandren

johanandren Apr 11, 2016

Member

I'd vote for that, but it seems I am already in minority.

Member

johanandren commented Apr 11, 2016

I'd vote for that, but it seems I am already in minority.

@rkuhn

This comment has been minimized.

Show comment
Hide comment
@rkuhn

rkuhn Apr 11, 2016

Collaborator

Then I’ll come to your aid: when reading the English text it is “either String or Int”, in that order.

Collaborator

rkuhn commented Apr 11, 2016

Then I’ll come to your aid: when reading the English text it is “either String or Int”, in that order.

@rkuhn

This comment has been minimized.

Show comment
Hide comment
@rkuhn

rkuhn Apr 11, 2016

Collaborator

(Even though the resulting semantics may be surprising due to wanting the Left case be a catch-all type. Yes, this fact might invalidate the approach ;-) )

Collaborator

rkuhn commented Apr 11, 2016

(Even though the resulting semantics may be surprising due to wanting the Left case be a catch-all type. Yes, this fact might invalidate the approach ;-) )

extends RuntimeException(
s"Failed to unmarshal Either[${Logging.simpleName(leftClass)}, ${Logging.simpleName(rightClass)}] (attempted ${Logging.simpleName(rightClass)} first). " +
s"Right failure: ${right.getMessage}, " +
s"Left failure: ${left.getMessage}")

This comment has been minimized.

@2beaucoup

2beaucoup Apr 11, 2016

Contributor

Include value here?

@2beaucoup

2beaucoup Apr 11, 2016

Contributor

Include value here?

This comment has been minimized.

@ktoso

ktoso Apr 11, 2016

Member

Hm... yeah worth considering, the error starts getting a bit long but maybe that's fine

@ktoso

ktoso Apr 11, 2016

Member

Hm... yeah worth considering, the error starts getting a bit long but maybe that's fine

This comment has been minimized.

@ktoso

ktoso Apr 11, 2016

Member

Since you're in this PR – any opinion on order of evaluating? Right Left or Left Right?

@ktoso

ktoso Apr 11, 2016

Member

Since you're in this PR – any opinion on order of evaluating? Right Left or Left Right?

This comment has been minimized.

@2beaucoup

2beaucoup Apr 11, 2016

Contributor

I'd find it less intuitive if we switch so my vote would be to keep the current version.

@2beaucoup

2beaucoup Apr 11, 2016

Contributor

I'd find it less intuitive if we switch so my vote would be to keep the current version.

This comment has been minimized.

@ktoso

ktoso Apr 11, 2016

Member

😕 now I'm stuck... I like the Left being "fallback" too as witnessed here, feels more correct even if perhaps less "reads like english" hm.Will try to brainstorm this with the team in the morning.

@ktoso

ktoso Apr 11, 2016

Member

😕 now I'm stuck... I like the Left being "fallback" too as witnessed here, feels more correct even if perhaps less "reads like english" hm.Will try to brainstorm this with the team in the morning.

This comment has been minimized.

@ktoso

ktoso Apr 11, 2016

Member

Thanks for your input!

@ktoso

ktoso Apr 11, 2016

Member

Thanks for your input!

This comment has been minimized.

@2beaucoup

2beaucoup Apr 12, 2016

Contributor

No prob. One more argument would be that the happy (right) state should me more frequent than the exceptional (left) one so trying right first should be more efficient as it has a higher probability of success. Just my 2 cents.

@2beaucoup

2beaucoup Apr 12, 2016

Contributor

No prob. One more argument would be that the happy (right) state should me more frequent than the exceptional (left) one so trying right first should be more efficient as it has a higher probability of success. Just my 2 cents.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Apr 11, 2016

Member

Then I’ll come to your aid: when reading the English text it is “either String or Int”, in that order.

This helps.

Even though the resulting semantics may be surprising due to wanting the Left case be a catch-all type. Yes, this fact might invalidate the approach ;-)

... but this brings us back to square one again 😉

Member

ktoso commented Apr 11, 2016

Then I’ll come to your aid: when reading the English text it is “either String or Int”, in that order.

This helps.

Even though the resulting semantics may be surprising due to wanting the Left case be a catch-all type. Yes, this fact might invalidate the approach ;-)

... but this brings us back to square one again 😉

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Apr 11, 2016

Member

If I count correctly we're 2:2 now?

Member

ktoso commented Apr 11, 2016

If I count correctly we're 2:2 now?

@johanandren

This comment has been minimized.

Show comment
Hide comment
@johanandren

johanandren Apr 11, 2016

Member

I have a Dr on my side though.

Member

johanandren commented Apr 11, 2016

I have a Dr on my side though.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Apr 11, 2016

Member

Is he on the Right side though? </end-of-these-jokes>

Ok, all jokes aside I guess Left then Right does make sense when one reads it, and it was also counter intuitive for me while implementing it. I corrected my thinking due to knowing how Either usually is used, however keyword "corrected" here, as intuitively I expected Left then Right when I first looked at it.

Member

ktoso commented Apr 11, 2016

Is he on the Right side though? </end-of-these-jokes>

Ok, all jokes aside I guess Left then Right does make sense when one reads it, and it was also counter intuitive for me while implementing it. I corrected my thinking due to knowing how Either usually is used, however keyword "corrected" here, as intuitively I expected Left then Right when I first looked at it.

@johanandren

This comment has been minimized.

Show comment
Hide comment
@johanandren

johanandren Apr 12, 2016

Member

Could it be that the ambiguity points to having it in akka http not being a good idea, since how you expect it to work is different from person to person? I think we should prefer things that are more obvious in the API (and it is already a pretty big public surface area)

Member

johanandren commented Apr 12, 2016

Could it be that the ambiguity points to having it in akka http not being a good idea, since how you expect it to work is different from person to person? I think we should prefer things that are more obvious in the API (and it is already a pretty big public surface area)

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Apr 22, 2016

Member

I'm going to merge this, it's useful as is and if people are confused about order of things we can revisit that.

Member

ktoso commented Apr 22, 2016

I'm going to merge this, it's useful as is and if people are confused about order of things we can revisit that.

@ktoso ktoso merged commit c57c6ed into akka:master Apr 22, 2016

2 checks passed

default Test PASSed. No test results found.
Details
typesafe-cla-validator All users have signed the CLA
Details

@ktoso ktoso deleted the ktoso:wip-either-unmarshalling-ktoso branch Apr 22, 2016

@johanandren

This comment has been minimized.

Show comment
Hide comment
@johanandren

johanandren Apr 22, 2016

Member

LGTM (after the fact)

Member

johanandren commented Apr 22, 2016

LGTM (after the fact)

@jmzhang

This comment has been minimized.

Show comment
Hide comment
@jmzhang

jmzhang Feb 26, 2017

I was trying to unmarshal a string to an either, but a bit surprised to discover the current eitherUnmarshaller is only available for HttpEntity. Any chance it could be made even more generic?

jmzhang commented Feb 26, 2017

I was trying to unmarshal a string to an either, but a bit surprised to discover the current eitherUnmarshaller is only available for HttpEntity. Any chance it could be made even more generic?

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