-
-
Notifications
You must be signed in to change notification settings - Fork 345
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
Add some helpers to simplify cross-version/cross-platform modules #2406
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the Wrapper
trait name hard to grasp. Maybe we can find some better name for it? And in the example we should definitely use some example domain name instead of wrapper
, to make sure, the names are not bound. (They are just coincidentally named "wrapper".)
Otherwise, @lolgab is using multi-platfrom-cross-scala project more frequently than me and also has put some thought and work (a dedictated plugin for that purpose) into this topic, so I'd value his opinion here!
Haven't looked at your solution yet but, if you haven't yet, I suggest you to give a look at my plugin's code: https://github.com/lolgab/mill-crossplatform By the way, I will review the PR later :) |
@lolgab looking at your repo, it seems your implementation has the advantage of adding a I can try porting over that functionality into |
Yeah I don't like the name either, but I don't know what else to call it. |
If there's no other feedback, I'm planning on merging this tomorrow. This is a relatively spartan implementation, and somewhat different from https://github.com/lolgab/mill-crossplatform, but I think provides enough value as is. We can continue to refine the code in |
@lihaoyi Can we find a better name for the I'm heavily time-pressured right now and your lately pushing, although very appreciated, is forcing me into reaction mode. I find it hard to come up with some better name, but I guess any other proposal containing the words "cross", "base" or "template" in it are already better that "wrapper". |
@lefou how about |
That will work. |
Updated the name, will probably merge tomorrow if CI is happy |
This PR attempts to minimize the boilerplate of defining cross-version cross-platform Scala modules.
Currently, cross-version cross-platform modules involves a lot annoying fiddling with overriding
millSourcePath
andsources
andartifactName
and passing aroundcrossScalaVersion
and all that. SimpleCross
/CrossScalaModule
s get some of the plumbing handled behind the scenes, but anything more complicated and the plumbing spills out again into plain view. And althoughCross
/CrossScalaModule
s have less boilerplate, it's still enough boilerplate you don't want to go around defining them over and overThis PR tries to hide all of them behind helper traits
CrossScalaModule.Wrapper
andPlatformScalaModule
, so your build file only has the logic you care about and none of the plumbing.Major changes
We replace the
artifactName
definition, so that instead of reading frommillModuleSegments
directly, it reads fromartifactNameParts: Seq[String]
. This makes it easier for us to splice the artifact name parts to remove unwanted segments without needing to do fragile string manipulationIntroduce
CrossScalaModule.Wrapper
. This can be used when theScalaModule
s you are defining are not the direct children of theCross
module, but are nested somewhere inside of it.CrossScalaModule.Wrapper
shades theCrossScalaModule
trait, so that anyCrossScalaModule
defined within its body would have thecrossScalaVersion
automatically wired up and theartifactNameParts
properly adjusted to remove the cross segment in the middle of the artifact nameIntroduce
PlatformScalaModule
. This is analogous toCrossScalaModule
, in that it adjusts themillSourcePath
/artifactNameParts
to drop the last segment, and adjustssources
to include the-{jvm,js,native}
suffixTests
The changes are largely covered by existing tests, including the
example/
tests which were adjusted to make use of the new traitsNotes
We cannot make the platform-specific sub-modules into
Cross
modules like we do with the version-specific sub-modules, since the platform-specific submodules have pretty different types/targets defined on them andCross
modules must all be of the same type6-cross-platform-publishing
, is a lot better than before, but there is still a significant amount of boilerplate involved, especially the lines aroundobject wrapper
. Not sure if there are other things we can do to make it even easier to define these cross-version/platform modules