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

Issue225 #259

Merged
merged 18 commits into from May 2, 2019

Conversation

Projects
None yet
2 participants
@tomasherman
Copy link
Contributor

commented Apr 27, 2019

Hey guys,

I took a stab at #225 ... what do you think? There might be some corners to polish but do you feel like the general strokes are correct?

Tomas Herman added some commits Apr 27, 2019

Tomas Herman
Tomas Herman
Tomas Herman
Tomas Herman
@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

i changed the implementation, pls take a look ... Also i'm wondering if we should make some sort of case class that describes all possible arguments to class that does the pattern matching, as the handlerWrapper and tracing signature is quite big and kinda confusing (a lot of function literals) ... maybe if we had some:

case class HandlerArguments(
  handlerWrapper: (String, Request[F], F[Response[F]]) => Response[F],
  tracer: ...
)

and than just pass that to the class, it would make for cleaner code ... what do you think @blast-hardcheese ?

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Apr 28, 2019

I hesitate to do it unless there just get to be way too many parameters. Two or three parameters isn't enough motivation for me right now, unless you're trying to solve a particular problem?

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

i'm not ... with that said, i don't have nothing more to add to this PR unless you have any other comments. So feel free to merge this if it makes sense to you

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Apr 29, 2019

One last note, please update MIGRATING.md with migration notes for this change, as it will require syntax changes for folks that are using tracing.

I'm also debating on default parameters here, vs overloaded constructors.

  • Default values require no multiple constructor juggling, and can be auto-curried
  • Overloaded constructors can permit both apply(tracing), apply(observeRoute), and apply(observeRoute, tracing), making migration to the new style a deprecation warning instead of "incompatible types"

I'm also debating on whether tracing should come before observeRoute or after.

  • After makes sense, as every generated resource will have this parameter
  • Before makes sense, observeRoute has a default value, whereas traceBuilder does not
@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@blast-hardcheese I don't disagree. The reason why i chose to put tracing before routeObserve is to make source code backward compatible. I don't have strong feelings either way so i decided to just choose working solution that is backward compatible.

If you reach different conclusion, i am willing to change it (altho adding multiple constructors will be more difficult, but probably doable). Let me know what you want me to do

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

when we make up our mind about it, i will fill in the migration guide ... also we will nneed something like this for client side also but we can do that as separate issue, server side is more important

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

one other consideration - how would you feel about introducing a trait for observeRoute? The method signature is quite long and it might make sense to add trait or at least type alias for it, perhaps into companion object of generated class?

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

Also, how do you feel about putting route body into

val response = { $routeBody}

and then generate the val name response in such a way so that it doesn't conflict with path params? I think we will need to do that in order to easily apply the observeRoute, because you need to pass it into the method and also eval it and return it

@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

So i thought about this again :)

if we decide to go with this:

observe: (String, Request[F], F[Response[F]] => F[Unit]

than there is no way to run it, because

val response: F[Response] = ???
observe(str, req, response) <* response

will run response twice

so we either need to return F[Response[F]] from the observe or change the signature to:
observe: (String, Request[F], F[Response[F]] => F[Unit]
and run it like:

val response: F[Response] = ???
response.flatMap(r => observe(str, req, r).map(_ => r))

which is ok, but a bit limiting, because it for example won't let you measure duration of the response processing ... so i would suggest to move back, again, to returning F[Reponse[F]] from the obesrve method ... thoughts?

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented Apr 30, 2019

Yeah... not flatMapping would permit more analysis. I guess the best thing would be to go with the types of the middleware from http4s, to greatly increase code reuse.

Adapting existing middleware could be as simple as middleware.contramap[(String, Request[F], F[Response[F]])](_._3) if they don't need additional functionality.

OK. Rename to mapRoute and I'm onboard.

tomasherman and others added some commits Apr 30, 2019

Tomas Herman
@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

So i gave a try to implementing multiple constructors and it's not trivial ... i would suggest to move this into a different issue as it will require some refactoring across the board ... for example i would suggest adding Constructor into language abstraction and than use those constructs, instead of extraRouteParameters

But i don't really have the time to go into it right now and i think the solution with default value for mapRoute is good enough ... what do you think?

Tomas Herman added some commits May 1, 2019

Tomas Herman
Tomas Herman
@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

ok so this is my latest attempt ... i agree that multiple constructors would be better solution but i think it warrants bigger refactoring that is not in scope of this PR ... i would be really happy if we could merge this without that. It can come later, perhaps.

With that said, i chose to use the default value in order to generated code backward compatible

Tomas Herman added some commits May 2, 2019

Tomas Herman
Tomas Herman
@tomasherman

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

ok i believe this one is ready :)

@blast-hardcheese blast-hardcheese referenced this pull request May 2, 2019

Closed

Add security support for Dropwizard #274

0 of 1 task complete
@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator

commented May 2, 2019

Thanks!

@blast-hardcheese blast-hardcheese merged commit 87b68e7 into twilio:master May 2, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tomasherman tomasherman deleted the avast:issue225 branch May 2, 2019

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