Move JDK shims to munit.internal

[armanbilge]

Will fix #437.

[olafurpg]

Thank you for this contribution! Looks good to me overall, just a few nitpick comments on undocumented conventions I try to keep in the codebase.

[olafurpg]

Let's call this MUnitPath instead of Path. Or something else if you have suggestions. I don't like it when libraries use generic names for internal classes because it pollutes the namespace when you're auto-completing symbols.

[armanbilge]

Makes sense, done.

[olafurpg]

Let's introduce a new munit.internal.io package. The general structure I try to following is

• flat list of classes/objects inside munit._ for public API
• flat list of packages inside munit.internal._

The reason is mostly historical because MUnit includes a reimplementation of the java difflib and it didn't feel right to have all of those diff classes inside munit.internal._.

[armanbilge]

Done.

[olafurpg]

Can we add an object called PlatformIO that has these methods and aliases instead? I try to avoid package objects when possible.

[armanbilge]

I moved the io-related ones to PlatformIO. I moved the reflect ones to PlatformCompat.

[olafurpg]

You can run scalafixAll and scalafmtAll inside the sbt shell to fix the Scalafix error

[armanbilge]

Thanks for the feedback and pointers. I'm still tracking down various linking issues for JS then I'll iterate on your feedback 👍

[olafurpg]

Just a heads up, I've been thinking how to solve another issue #382 and just realized it's somewhat related to this issue. The root problem is that MUnit does file I/O at runtime, which is incompatible with JS in the browser and distributed build tools like Gradle and Bazel.

One alternative solution to this problem is to embed the full text contents of the file inside munit.Location. I avoided this approach in the past because I was concerned it would make the generated code too big (because every Location would have a fresh copy of the full source code). However, the string literal gets added to a constant pool so it only stored once. I validated that this is the case for JVM bytecode and Scala 3 tasty, but I haven't confirmed whether this is also true for Scala.js/Native IRs (I would be surprised if they don't have a constant pool).

[armanbilge]

Uh-oh, maybe I should wait on this until you decide what you want to do? 😅 Removing I/O completely would be excellent for browsers and friends.

[olafurpg]

You can try to implement this alternative approach if you're interested. The steps would be

1. Add new textContents field to munit.Location, with a fallback constructor that defaults to textContents = ""

   munit/munit/shared/src/main/scala/munit/Location.scala

   Line 14 in e331ee3

   val line: Int

2. Update the Scala 2 macro to inline the contents here

   munit/munit/shared/src/main/scala/munit/internal/MacroCompatScala2.scala

   Line 15 in e331ee3

   New(c.mirror.staticClass(classOf[Location].getName()), path, line)

3. Update the Scala 3 macro to inline the contents here

   munit/munit/shared/src/main/scala-3/munit/internal/MacroCompat.scala

   Line 20 in e331ee3

   '{ new Location(${Expr(path)}, ${Expr(startLine)}) }

4. Update Lines.scala to use Location.textContents instead of file I/O

[olafurpg]

I'm sorry for guiding you in the other direction, this idea didn't come up until after you opened the draft PR.

[olafurpg]

The CI seems green so we can also merge this PR as is and I can look into the alternative later.

[armanbilge]

Apologies, getting a bit out-of-scope for me. As it stands this PR does the hard part of #437 by tracking down all the JDK stuff and condensing it in the internal package. All that's left is bike-shedding names according to unwritten convention which it seems you are the expert on 😉

[olafurpg]

All that's left is bike-shedding names according to unwritten convention which it seems you are the expert on

It's not bike-shedding IMO to follow conventions of existing code structure. I apologize if it seems like a waste of time or if your dissatisfied with the lack of documentation for how the code should be structured. I'm happy to merge this PR as is once the proposed renames and code structure suggestions have been implemented.

[olafurpg]

I don't claim these conventions are the best conventions or that they're even standard conventions. They're just conventions that are part of this codebase, for better or worse (some of them are accidental while others are for historical reasons).

[armanbilge]

Apologies, I regret my remark as it came across poorly I think 😕 I have ton of respect for projects and developers that keep things organized, extremely important for projects like this at the top of the dependency food chain. It's just hard for me to know exactly how you want things without a couple more iterations and it seems the future of this PR is dubious depending on some very good changes that might be forthcoming. So I'm happy to let this sit and if it's still relevant by the time 1.0 is solidifying then we can make it all perfect :)

[olafurpg]

Sound good, we can let this PR wait. No worries!

[olafurpg]

FYI I'm working on this issue here right now #41 and it may take a while to get through the finish line.

[armanbilge]

@olafurpg we've been butting heads with this problem again in http4s/http4s#5758.

Would you be open to accepting the changes in this PR into 0.7.x series (after it's polished up)? I know it's technically a breaking change, but in fact anyone who experiences that breaking change likely had bugs/linking issues that were masked precisely because of these shims.

Thanks!

[armanbilge]

Oh, one more idea: if you really want to avoid a breaking change, we apply the changes here, and also move these shims into a new artifact. So anyone that experiences issues because of the change, can just add a new dependency to get the shims back (or alternatively, the shims can be included by default and downstreams where it causes a problem can remove that artifact explicitly).

[armanbilge]

@olafurpg I've now renamed/re-organized everything as you suggested. Thanks for your patience on this one! :)

[olafurpg]

LGTM thank you!