-
Notifications
You must be signed in to change notification settings - Fork 497
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
Feature multi project support #1875
Feature multi project support #1875
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1875 +/- ##
==========================================
+ Coverage 75.44% 75.49% +0.05%
==========================================
Files 128 129 +1
Lines 2158 2179 +21
Branches 55 57 +2
==========================================
+ Hits 1628 1645 +17
- Misses 530 534 +4
Continue to review full report at Codecov.
|
modules/core/src/main/scala/org/scalasteward/core/buildtool/maven/MavenAlg.scala
Outdated
Show resolved
Hide resolved
modules/core/src/main/scala/org/scalasteward/core/vcs/data/BuildRoot.scala
Outdated
Show resolved
Hide resolved
…nto feature-multi-project-support
trait BuildToolDispatcher[F[_]] extends BuildToolAlg[F] { | ||
def runMigrationsForAllBuildRoots(repo: Repo, migrations: Nel[Migration]): F[Unit] | ||
def getDependenciesForAllBuildRoots(repo: Repo): F[List[Scope.Dependencies]] | ||
} |
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 it unfortunate that BuildToolDispatcher
has now methods that work with Repo
and BuildRoot
but the only methods used outside of BuildToolDispatcher
are the ones with Repo
. Have you tried to add a second type parameter to BuildToolAlg
as mentioned in my comment:
BuildToolDispatcher and the build tool traits like SbtAlg inherit from BuildToolAlg which uses Repo for its methods. Since SbtAlg, MavenAlg, and MillAlg should now operate on BuildRoots instead of Repo but BuildToolDispatcher should still use Repo we need to add a new type parameter to the BuildToolAlg, i.e. BuildToolAlg[F, R] where R would be either a Repo or a BuildRoot.
?
I think with this change, the above additions aren't necessary and the BuildToolDispatcher
's callsites (in EditAlg
and RepoCacheAlg
) also do not need to be changed.
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.
Right that might work, I totally didn't see that part of the comment somehow. I'll change this asap.
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.
Switched over, yeah that is a much nicer approach. Technically it should be possible to have contravariant typeclasses on BuildToolAlg
that would allow the dispatcher to work easily both for Repo
and BuildRoot
I didn't do that since I'm a bit pressed for time now but if you see any value in that I'll happily change it to that.
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.
Looks good to me now, thanks. I think there is no need for more changes since the dispatcher is only used with Repo
for now.
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.
LGTM. Thanks @GrafBlutwurst!
This is a much simpler attemt to get support for multiple projects within a mono repo based on the feedback here:
#1093 (comment)
Currently it's still missing tests since I haven't had a good idea how to do those without unnecessary codebloat.