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

Allow loading GlobalResources from libraries on the classpath. #494

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aevanszen
Copy link

As discussed in #479.

This is a minimal implmentation to allow GlobalResources to be reused in a submodule / external jar across projects.

I've tested on a local project and it looks to work as expected.

This is useful if you want to package GlobalResource up as a subproject and re-use in a multi-module build.
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@@ -184,6 +187,13 @@ trait RunnerCompat[F[_]] { self: sbt.testing.Runner =>
globalResources.traverse(_.sharedResources(map)).void
}

private def spiGlobalResources: List[GlobalResourceF[F]] = {
val result = ListBuffer.empty[GlobalResourceF[F]]
val loader: ServiceLoader[GlobalResourceF[F]] = ServiceLoader.load(classOf[GlobalResourceF[F]])
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @aevanszen, thanks for the PR.

I gave it some long thought, and the problem with doing it this way is type-erasure. Even if it's unlikely that people would use different effect types in a single codebase, I'd be keen to protect against it. Because GlobalResourceF is parameterised, the load call would return everything, whether it's using cats-effect, monix or zio, and obviously loading monix globals in a CE context wouldn't be great.

So what I think we should do instead, would be for the ServiceLoader call to be abstract away. We have a SuiteLoader abstraction, and we could add a def loadGlobalResources to it.

The implementation of SuiteLoader lives there, at a place that is aware of the classes that the test-framework is supposed to understand. Those are effect-type specific (there is one set defined for CE, one for monix, one for zio etc).

Now the thing is that you'll need one other layer of indirection to support cross-compilation. IE you could have a PlatformServiceLoader object defined in a src-jvm, containing a spiGlobalResources[T](cl : ClassLoader, cls: Class[T]) : List[T] method (similar to what you've implemented here), and a similar object defined in src-js , that'd have a method with the same signature but would return an empty list.

This way, type-erasure would be accounted for.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the detailed reply @Baccata, the suggestions are very helpful.

I had the same reservations around type-erasure but wasn't sure the best way to work around it in the framework just yet. Your suggestion is a good starting point, and I'll look into it.

I'll add the tests and documentation when the implementation is ok and update the PR with the changes.

The GlobalResoure classpath isn't an urgent blocker for me, so updating this PR may be slow as I work on it when I get the spare time. I will try to work on it, though, as it'll be useful to have.

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem whatsoever, I certainly appreciate the contribution, even if it takes time. There is no rush :) let us know if you get stuck or if you want us to have a look at some commits

@Baccata
Copy link
Contributor

Baccata commented Mar 7, 2022

I'd also appreciate :

  • a test (though I don't mind if we defer, unless you want to take a stab at it).
  • an amendment to the documentation page to describe the SPI-based mechanism

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants