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 Directive#tcollect and Directive1#collect #2253

Merged
merged 1 commit into from Dec 13, 2018

Conversation

Projects
None yet
5 participants
@markus1189
Copy link
Contributor

markus1189 commented Oct 11, 2018

This commit adds two new methods on Directives:

  • Directive#ttransform the tupled variant of Future#transform that
    allows mapping and filtering at the same time
  • Directive1#transform the equivalent of Future#transform for
    Directive1, the specialized version of Directive#ttransform

As an example, instead of writing:

parameter("x".as[Int]).filter(x => x >= 0, someRejection).map(x => Positive(x))

we can now write this in one go using Directive1#transform:

parameter("x".as[Int]).transform({ case x if x >= 0 => Positive(x) }, someRejection)

Review Notes:

  • composing PartialFunctions is really awkward, had to hack around it in Directive1#transform because there we have to nest two of them
  • I would prefer a curried version of transform that takes the partial function as the second argument because it looks better, but I went with the two-parameter style as done in Directive1#filter
@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Oct 11, 2018

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!

@markus1189 markus1189 force-pushed the markus1189:directive-transform branch from 38c7e77 to 7981986 Oct 12, 2018

@markus1189 markus1189 changed the title Add Directive#ttransform and Directive1#transform Add Directive#tcollect and Directive1#collect Oct 12, 2018

@markus1189

This comment has been minimized.

Copy link
Contributor

markus1189 commented Oct 12, 2018

Changed the name to collect because transform didn't fit as well.

@jlprat

This comment has been minimized.

Copy link
Member

jlprat commented Oct 12, 2018

OK TO TEST

@akka-ci akka-ci added validating tested and removed validating labels Oct 12, 2018

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Oct 12, 2018

Test PASSed.

@johanandren
Copy link
Member

johanandren left a comment

Looks nice, and I agree collect fits better with that it does, should it be mentioned in the reference docs page about composing directives also?

@jlprat

This comment has been minimized.

Copy link
Member

jlprat commented Oct 12, 2018

I guess we would also need the Java counterpart for it.

@markus1189

This comment has been minimized.

Copy link
Contributor

markus1189 commented Oct 12, 2018

@johanandren I will have a look at the docs

@jlprat Where would you add that in the codebase? For example there seems to be no tmap in the Java API anyway (according to my grep skills 😅 )

@markus1189

This comment has been minimized.

Copy link
Contributor

markus1189 commented Oct 12, 2018

@johanandren I agree that it would be good to have something in the docs, but I couldn't find mentions of tmap, tflatMap etc., so that covering only tcollect would be strange.

Maybe at another point and another pr?

@jlprat

This comment has been minimized.

Copy link
Member

jlprat commented Oct 12, 2018

@markus1189 The Java API works quite differently and some times is not possible to mimic the same API for both languages.
Some times it can be done in this manner: https://github.com/akka/akka-http/blob/master/akka-http/src/main/scala/akka/http/javadsl/server/Directives.scala#L44
Some times, there is no general way or directly it makes no sense in Java.

@johanandren

This comment has been minimized.

Copy link
Member

johanandren commented Oct 12, 2018

Yeah, I was under the impression that we had more docs on it in this section https://doc.akka.io/docs/akka-http/current/routing-dsl/directives/custom-directives.html but I see that we really don't so you can skip that then.

@johanandren

This comment has been minimized.

Copy link
Member

johanandren commented Oct 12, 2018

Too quick on the comment button, now I found at least flatMap and tflatMap in that section: https://doc.akka.io/docs/akka-http/current/routing-dsl/directives/custom-directives.html#flatmap-and-tflatmap

So adding a small section after that with some text about when it is useful to do collect and a little sample would be nice.

Add Directive#tcollect and Directive1#collect
This commit adds two new methods on `Directives`:

- Directive#tcollect the tupled variant of Future#collect that
  allows mapping and filtering at the same time
- Directive1#collect the equivalent of, e.g., List#collect for
  Directive1, the specialized version of Directive#tcollect

As an example, instead of writing:

```
parameter("x".as[Int]).filter(x => x >= 0, someRejection).map(x => Positive(x))
```

we can now write this in one go using Directive1#collect:

```
parameter("x".as[Int]).collect({ case x if x >= 0 => Positive(x) }, someRejection)
```

@markus1189 markus1189 force-pushed the markus1189:directive-transform branch from 7981986 to 4f692ca Oct 12, 2018

@markus1189

This comment has been minimized.

Copy link
Contributor

markus1189 commented Oct 12, 2018

@johanandren I added a section about collect ;)

@jlprat I don't see things like map in the Java file you posted. I would assume that if that is not implemented it would not make sense to implement collect either?

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Oct 12, 2018

Test PASSed.

@johanandren

This comment has been minimized.

Copy link
Member

johanandren commented Oct 12, 2018

There is a akka.japi.pf.PFBuilder to build partial functions in Java but I think we are trying to avoid using that nowadays.

@jlprat

jlprat approved these changes Nov 19, 2018

Copy link
Member

jlprat left a comment

LGTM!

Thanks @markus1189

@jlprat jlprat added the 2 - pick next label Dec 6, 2018

@jrudolph
Copy link
Member

jrudolph left a comment

LGTM, thanks @markus1189.

@jrudolph jrudolph merged commit 2f8694f into akka:master Dec 13, 2018

3 checks passed

Jenkins PR Validation Test PASSed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@jrudolph jrudolph added this to the 10.1.6 milestone Dec 13, 2018

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