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

Covary F in Message types. #3107

Merged
merged 3 commits into from Jan 25, 2020
Merged

Covary F in Message types. #3107

merged 3 commits into from Jan 25, 2020

Conversation

zarthross
Copy link
Member

@zarthross zarthross commented Jan 23, 2020

Adding a covary function to Message and its subtypes that matches the Stream.covary function.

It's implemented as a type cast to avoid the costs of creating a new Request object for something that should be safe to do as a cast.

Alternative implementation without a type cast, (but including an extra allocation of Request is here

Copy link
Member

@rossabaker rossabaker left a comment

I thought we had this already. I prefer this to the allocating version.

To gain confidence, we could try this in a spec with a pair of effects that work, and using illTyped, one that doesn't.

@rossabaker rossabaker added this to the 0.21.0-RC2 milestone Jan 23, 2020
@rossabaker
Copy link
Member

@rossabaker rossabaker commented Jan 23, 2020

Out of curiosity, do you need this for rho, or other reasons?

@rossabaker rossabaker added the enhancement label Jan 23, 2020
@zarthross
Copy link
Member Author

@zarthross zarthross commented Jan 23, 2020

I'll write up some spec's later, wanted to get buy in before putting in the effort.

I didn't need it for Rho.

Copy link
Contributor

@hamnis hamnis left a comment

Would this be useful on Media ?

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Jan 23, 2020

Good call, @hamnis. It would thread through Part then, too.

@diesalbla
Copy link
Contributor

@diesalbla diesalbla commented Jan 24, 2020

Rather than adding a covary method, can we just make the type parameter covariant?

sealed trait Message[+F[_]]

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Jan 24, 2020

Rather than adding a covary method, can we just make the type parameter covariant?

sealed trait Message[+F[_]]

Maybe. It would probably want to start in Media. I expect some operations that take typeclass constraints would have to move to something like fs2's InvariantOps (e.g., as, attemptAs). I don't know what other fallout there would be. Do you want to poke at it and see how sweeping the change is?

@diesalbla
Copy link
Contributor

@diesalbla diesalbla commented Jan 24, 2020

Sure. Do you mind keeping this PR on hold for a little while, until I have tried those changes?

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Jan 24, 2020

We're already in an RC cycle, with hopes of the release next week that many people have been awaiting for too long. My inclination is to proceed with @zarthross' approach in 0.21, and consider the covariance in the next release.

@rossabaker rossabaker added breaking and removed enhancement labels Jan 24, 2020
@zarthross
Copy link
Member Author

@zarthross zarthross commented Jan 24, 2020

Per feedback, I've added covary up to Media so its now on Part as well, and I've added some specs that are hopefully sufficient.

Copy link
Member

@rossabaker rossabaker left a comment

Nice work. I'll open a separate ticket so we don't lose @diesalbla's idea for the future, but I think this solves the problem now.

The test failures are unrelated. Two of the newer ones are acting flaky on us.

@rossabaker rossabaker merged commit 28945bb into http4s:master Jan 25, 2020
2 checks passed
@zarthross zarthross deleted the Messsage-Covary branch Jan 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants