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

Refactor Http4sMatchers to be generic on F-Type #2080

Merged
merged 3 commits into from Sep 13, 2018

Conversation

Projects
None yet
5 participants
@diesalbla
Contributor

diesalbla commented Sep 12, 2018

Resolves #1430.

As described in issue #1430, the Http4sMatchers trait is not generic on the effect F[_] type, and neither was a parent trait IOMatchers trait. We try to address this situation doing the following:

  • We factor out the trait IOMatchers into a RunTimedMatchers trait, which keeps most fo the IOMatchers trait methods and logic, but is generic on the effect type F[_]. Since IOMatchers was using some specific methods of cats.effect.IO, we need to extract those as abstract method, following the GoD template-method pattern.
  • We turn IOMatchers into an extension of RunTimedMatchers that specialises those abstract methods, to the methods of IO.
  • We make Http4sMatchers generic on the F type. Luckily, this is just replacing "IO" with F

Other issues and related or future work.

One thing to note is that, in the RunTimedMatchers, we need to declare F-generic abstractions for two methods in the IO class:

  • unsafeRunSync, which takes an IO[A] and returns an A (or throws an exception); and
  • unsafeRunTimed, which takes an IO[A] and a timeout, and either returns an A in time or throws a timeout error.

These methods have no type-class capturing them, although an issue was opened in cats-effect that proposed creating a type-class that would have included the first method: typelevel/cats-effect#230

It should be noted that, since the existing design is a lattice of inheritance and mixins, the solution only adds two more nodes to that lattice. While it may be possible to replace the design by one with less inheritance, which would avoid the inner (path-dependent) class TimedMatcher, that would exceed the scope of the original ticket IMHO.

Refactor Http4sMatchers to be generic on F-Type
As described in issue #1430, the Http4sMatchers trait was not
parametrised on the Effect type. Neither was the parent trait
IOMatchers. To fix the situation, this commit does the following:

- We factor out the trait IOMatchers into a RunTimedMatchers trait,
  which keeps most fo the IOMatchers trait methods and logic, but is
  generic on the effect type `F[_]`.
  Since IOMatchers was using some specific methods of cats.effect.IO,
  we need to extract those (template method pattern).
  Note that this method do not correspond to any current type-class.

- We turn IOMatchers into an extension of RunTimedMatchers that
  specialises those abstract methods, to the methods of IO ,

- We make `Http4sMatchers` generic on the F type. Luckily, this is
  just replacing "IO" with F

- We add an `Http4sMatchersIO`, which unifies those traits. This will
  help for an easy migration for those previously using `Http4sMatchers`.
@SystemFw

This comment has been minimized.

Contributor

SystemFw commented Sep 12, 2018

unsafeRunSync, which takes an IO[A] and returns an A (or throws an exception); and

you can .toIO.unsafeRunSync on any ConcurrentEffect

@rossabaker

This comment has been minimized.

Member

rossabaker commented Sep 12, 2018

@rossabaker

specs2 has recently gained support for IOMatchers. The IOMatchers and RunTimedMatchers here extend specs2 but aren't HTTP-specific. I'm wondering if we should offer those traits to specs2-cats, and then restrict this just to the polymorphic Http4sMatchers.

Getting from here to there is complicated: specs2 now depends on scalacheck-1.14, but our laws dependencies are only published for scalacheck-1.13. I think this strategy is the right place to put things in the long term, but severely complicates getting this merged and released.

Thoughts?

@rossabaker

This comment has been minimized.

Member

rossabaker commented Sep 12, 2018

I'd also like to have an http4s-laws (no specs2 dependency) and an http4s-specs2 module for the matchers. Maybe that split could be coordinated with a scalacheck-1.14 upgrade and give us an excuse to adopt a specs2-owned trait, and we adopt these as a temporary fix.

@diesalbla

This comment has been minimized.

Contributor

diesalbla commented Sep 12, 2018

I agree that IOMatchers, should be removed from the http4s library in favour of specs2. Still, to achieve the goal of the issue and make Http4sMatchers generic, then we would also need the generic RunTimedMatchers in specs2. I will make a PR to port these changes.

As for the scalacheck dependency, in this PR we can just add an "excludeAll" in the import of specs2, and leave for another PR upgrading dependencies.

@rossabaker

This comment has been minimized.

Member

rossabaker commented Sep 13, 2018

I think the problem with excludeAll and upgrading to a new specs2 is that specs2's integration with scalacheck is in core, and will still be compiled against the binary incompatible 1.14. We'd be in a much better position with cats-laws and cats-effect laws on 1.14.

@diesalbla

This comment has been minimized.

Contributor

diesalbla commented Sep 13, 2018

@rossabaker I have opened a PR in specs2, to port there some changes to IOMatchers suggested here.

Looking through the build files of those projects, I have found a curious thing: cats uses different versions of scalacheck and other dependencies , depending on which version of the Scala compiler it is building with. Unless you want to bring this kind of conditional dependencies to http4s, I do not see how to move forward.

In any case, I humbly believe that upgrading test dependencies, and watching for any regression that may introduce, goes beyond the scope of the original issue.

@rossabaker

LGTM.

We're going to have a mess when Scala 2.13 forces us to straddle the scalacheck-1.13 vs. 1.14 divide. Hopefully we can get cats-laws and cats-effect-laws crossbuilt for 1.14 by that point. But we'll cross that bridge when we come to it: this is strictly an improvement where we're at today.

@jmcardon jmcardon merged commit df8ed50 into http4s:master Sep 13, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment