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 String to UUID unmarshaller to the predefined Scala unmarshallers #2505

Merged
merged 2 commits into from May 15, 2019

Conversation

@maximskripnik
Copy link
Contributor

commented Apr 10, 2019

Purpose

Add ability to unmarshall Strings to UUIDs in Scala code via predefined implicit Unmarshaller[String, UUID]

Background Context

So we have followings things already:

But we don't have similar thing for Scala Unmarshaller, so one can't do stuff like this (that is actual use case which I needed):

parameter('id.as[UUID]) { id => ... }

While you can do this:

path(JavaUUID) { id => ... }

Seemed like a good thing, so I was surprised that it is not available out of the box for Scala, but is for Java, hence this PR. If I just looked in the wrong place and it is actually somewhere there, please guide me through it

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Apr 10, 2019

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

@johanandren

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

OK TO TEST

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Apr 10, 2019

Test FAILed.

@maximskripnik

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@johanandren Does this really break binary compatibilty or is it safe to keep as is?

@johanandren

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

I think it does break bincomp and source compatibility because someone may have mixed in the PredefinedFromStringUnmarshallers trait and already implemented a method called uuidFromStringUnmarshaller.

I'm not sure if we have been historically strict with those. @jrudolph ?

@jrudolph
Copy link
Member

left a comment

LGTM with a small question

@jrudolph

This comment has been minimized.

Copy link
Member

commented May 14, 2019

I think it does break bincomp and source compatibility because someone may have mixed in the PredefinedFromStringUnmarshallers trait and already implemented a method called uuidFromStringUnmarshaller.

I'm not sure if we have been historically strict with those. @jrudolph ?

I don't think we need to be strict here. Maybe we should add @DoNotExtend to those classes for the future.

Can you add mima exclusions for these, @maximskripnik?

@maximskripnik

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

@jrudolph Sure, there those are

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2019

Test PASSed.

@jrudolph jrudolph merged commit 71eaf6e into akka:master May 15, 2019

4 checks passed

Jenkins PR Auto-Formatter Successful
Details
Jenkins PR Validation Test PASSed. 4202 tests run, 1145 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details
@jrudolph

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Great, thanks @maximskripnik.

@jrudolph jrudolph added this to the 10.1.9 milestone May 15, 2019

franktominc added a commit to franktominc/akka-http that referenced this pull request Jun 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.