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

httpRoutes constructor for AutoSlash middleware #3286

Conversation

voidcontext
Copy link
Contributor

@voidcontext voidcontext commented Mar 28, 2020

This PR is part of #2402.

I am opening this PR to start some discussions about the solution, although tests are passing (after 7-8 rerun of the github actions) but I am not sure about the implementation and the value of the feature.

Please see comments below.

def httpRoutes[F[_]: Monad](httpRoutes: HttpRoutes[F]): HttpRoutes[F] =
apply(httpRoutes)

def httpApp[F[_]: MonoidK: Functor](httpApp: HttpApp[F]): HttpApp[F] =
Copy link
Contributor Author

@voidcontext voidcontext Mar 28, 2020

Choose a reason for hiding this comment

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

I am not sure how valuable this constructor is, given the fact that F still should have a MonoidK instance and in this case F is usually IO which - at least as far as I know - doesn't have that.

Copy link
Contributor

@hamnis hamnis Apr 3, 2020

Choose a reason for hiding this comment

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

Agreed. this constructor is not useful

Copy link
Member

@rossabaker rossabaker Apr 26, 2020

Choose a reason for hiding this comment

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

Does it need to be a MonoidK, or just a SemigroupK? IO has one of those (as seen below), though whether it's useful is still dubious. "Try again with the slash if I raise an error," that would say.

Copy link
Member

@rossabaker rossabaker Apr 26, 2020

Choose a reason for hiding this comment

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

Oh, I see the F.empty in apply now. Yeah, I think we remove this, keep httpRoutes, and declare victory.

Copy link
Contributor Author

@voidcontext voidcontext Apr 27, 2020

Choose a reason for hiding this comment

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

Ok, I'll update this PR asap.


implicit val invalidMonoidKIO: MonoidK[IO] = new MonoidK[IO] {
def combineK[A](x: IO[A], y: IO[A]): IO[A] = IO.ioSemigroupK.combineK[A](x, y)
override def empty[A]: IO[A] = IO.raiseError(new Exception("error"))
Copy link
Contributor Author

@voidcontext voidcontext Mar 28, 2020

Choose a reason for hiding this comment

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

This MonoidK[IO] instance doesn't feel to be right, it serves the purpose of the test, but I don't think it's correct.

Copy link
Member

@rossabaker rossabaker Apr 26, 2020

Choose a reason for hiding this comment

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

Future.filter implies something like this (it throws a NoSuchElementException), but I think is a wart in the standard library. Requiring something like this is a great indication that what we're testing is not right.

@rossabaker rossabaker added enhancement Feature requests and improvements retarget Cherry-pick or reopen on another branch labels Apr 26, 2020
@rossabaker
Copy link
Member

rossabaker commented Apr 26, 2020

Sorry this one slipped through the cracks for a while. 0.20 is about to be EOL. If we hurry, we can put it there. Otherwise, it can be introduced in 0.21.

@voidcontext
Copy link
Contributor Author

voidcontext commented Apr 29, 2020

And sorry for the delay on my side as well. The httpApp constructor has beeen removed.

@rossabaker rossabaker added this to the 0.21.x milestone Apr 30, 2020
@hamnis hamnis changed the title WIP - httpRoutes and httpApp constructor for AutoSlash middleware httpRoutes constructor for AutoSlash middleware Apr 30, 2020
hamnis
hamnis approved these changes Apr 30, 2020
@rossabaker
Copy link
Member

rossabaker commented May 1, 2020

Backported onto series/0.21.

@rossabaker rossabaker closed this May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests and improvements retarget Cherry-pick or reopen on another branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants