Skip to content

Commit

Permalink
Refactor pull request head: replace conditionals with polymorphism
Browse files Browse the repository at this point in the history
This is another example of replacing conditionals with polymorphism on
`ForgeType`. This was a theme in commit b42866b, merged in this PR back
in August 2023:

scala-steward-org#3145

A benefit of this pattern is as new types of forge are implemented, the
responsibilities for supporting that new `ForgeType` are all in one
place, rather than spread across several methods elsewhere in the
codebase.

To confess, I think there's probably a further refactor that could be
done, to introduce a better parameter object denoting a 'Pull Request
Head' (that conceivably could better model the way that some
`ForgeType`s don't even support forking) - but that can wait for
another day, fixing scala-steward-org#3300 is more urgent!
  • Loading branch information
rtyley committed Feb 29, 2024
1 parent 4b02e6c commit 0e513d9
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 60 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,13 @@ import cats.syntax.all._
import org.http4s.Uri
import org.http4s.syntax.literals._
import org.scalasteward.core.application.Config.ForgeCfg
import org.scalasteward.core.data.Repo
import org.scalasteward.core.forge.ForgeType._
import org.scalasteward.core.git.Branch
import org.scalasteward.core.util.unexpectedString

import scala.annotation.nowarn

sealed trait ForgeType extends Product with Serializable {
def publicWebHost: Option[String]

Expand All @@ -41,6 +45,11 @@ sealed trait ForgeType extends Product with Serializable {
def supportsForking: Boolean = true
def supportsLabels: Boolean = true

/** Determines the `head` (GitHub) / `source_branch` (GitLab, Bitbucket) parameter for searching
* for already existing pull requests or creating new pull requests.
*/
def pullRequestHeadFor(@nowarn fork: Repo, updateBranch: Branch): String = updateBranch.name

val asString: String = this match {
case AzureRepos => "azure-repos"
case Bitbucket => "bitbucket"
Expand Down Expand Up @@ -94,6 +103,8 @@ object ForgeType {
val publicApiBaseUrl = uri"https://api.github.com"
val diffs: DiffUriPattern = (from, to) => _ / "compare" / s"$from...$to"
val files: FileUriPattern = fileName => _ / "blob" / "master" / fileName
override def pullRequestHeadFor(fork: Repo, updateBranch: Branch): String =
s"${fork.owner}:${updateBranch.name}"
}

case object GitLab extends ForgeType {
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import org.scalasteward.core.git.{Branch, Commit, GitAlg}
import org.scalasteward.core.repoconfig.PullRequestUpdateStrategy
import org.scalasteward.core.util.logger.LoggerOps
import org.scalasteward.core.util.{Nel, UrlChecker}
import org.scalasteward.core.{forge, git, util}
import org.scalasteward.core.{git, util}
import org.typelevel.log4cats.Logger

final class NurtureAlg[F[_]](config: ForgeCfg)(implicit
Expand Down Expand Up @@ -92,7 +92,7 @@ final class NurtureAlg[F[_]](config: ForgeCfg)(implicit
private def processUpdate(data: UpdateData): F[ProcessResult] =
for {
_ <- logger.info(s"Process update ${data.update.show}")
head = forge.headFor(config.tpe, data.fork, data.updateBranch)
head = config.tpe.pullRequestHeadFor(data.fork, data.updateBranch)
pullRequests <- forgeApiAlg.listPullRequests(data.repo, head, data.baseBranch)
result <- pullRequests.headOption match {
case Some(pr) if pr.state.isClosed && data.update.isInstanceOf[Update.Single] =>
Expand Down Expand Up @@ -230,20 +230,18 @@ final class NurtureAlg[F[_]](config: ForgeCfg)(implicit
.on(u => List(u.currentVersion.value), _.updates.map(_.currentVersion.value))
.flatTraverse(gitAlg.findFilesContaining(data.repo, _))
.map(_.distinct)
branchName = forge.headFor(config.tpe, data.fork, data.updateBranch)
allLabels = labelsFor(data.update, edits, filesWithOldVersion, artifactIdToVersionScheme)
labels = filterLabels(allLabels, data.repoData.config.pullRequests.includeMatchedLabels)
requestData = NewPullRequestData.from(
data = data,
branchName = branchName,
edits = edits,
artifactIdToUrl = artifactIdToUrl,
artifactIdToUpdateInfoUrls = artifactIdToUpdateInfoUrls.toMap,
filesWithOldVersion = filesWithOldVersion,
addLabels = config.addLabels,
labels = data.repoData.config.pullRequests.customLabels ++ labels
)
} yield requestData
} yield NewPullRequestData.from(
data = data,
branchName = config.tpe.pullRequestHeadFor(data.fork, data.updateBranch),
edits = edits,
artifactIdToUrl = artifactIdToUrl,
artifactIdToUpdateInfoUrls = artifactIdToUpdateInfoUrls.toMap,
filesWithOldVersion = filesWithOldVersion,
addLabels = config.addLabels,
labels = data.repoData.config.pullRequests.customLabels ++ labels
)

private def createPullRequest(data: UpdateData, edits: List[EditAttempt]): F[ProcessResult] =
for {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,27 +6,24 @@ import org.scalasteward.core.data.{Repo, Update}
import org.scalasteward.core.forge.ForgeType.{GitHub, GitLab}
import org.scalasteward.core.git

class ForgePackageTest extends FunSuite {
class ForgeTypeTest extends FunSuite {
private val repo = Repo("foo", "bar")

// Single updates

{

val update = ("ch.qos.logback".g % "logback-classic".a % "1.2.0" %> "1.2.3").single
val updateBranch = git.branchFor(update, None)

test("headFor (single)") {
assertEquals(headFor(GitHub, repo, updateBranch), s"foo:${updateBranch.name}")
assertEquals(headFor(GitLab, repo, updateBranch), updateBranch.name)
assertEquals(GitHub.pullRequestHeadFor(repo, updateBranch), s"foo:${updateBranch.name}")
assertEquals(GitLab.pullRequestHeadFor(repo, updateBranch), updateBranch.name)
}

}

// Grouped updates

{

val update = Update.Grouped(
name = "my-group",
title = None,
Expand All @@ -36,8 +33,8 @@ class ForgePackageTest extends FunSuite {
val updateBranch = git.branchFor(update, None)

test("headFor (grouped)") {
assertEquals(headFor(GitHub, repo, updateBranch), s"foo:update/my-group")
assertEquals(headFor(GitLab, repo, updateBranch), updateBranch.name)
assertEquals(GitHub.pullRequestHeadFor(repo, updateBranch), s"foo:update/my-group")
assertEquals(GitLab.pullRequestHeadFor(repo, updateBranch), updateBranch.name)
}
}

Expand All @@ -53,8 +50,8 @@ class ForgePackageTest extends FunSuite {
val updateBranch = git.branchFor(update, None)

test("headFor (grouped) with $hash") {
assertEquals(headFor(GitHub, repo, updateBranch), s"foo:update/my-group-1164623676")
assertEquals(headFor(GitLab, repo, updateBranch), updateBranch.name)
assertEquals(GitHub.pullRequestHeadFor(repo, updateBranch), s"foo:update/my-group-1164623676")
assertEquals(GitLab.pullRequestHeadFor(repo, updateBranch), updateBranch.name)
}
}

Expand Down

0 comments on commit 0e513d9

Please sign in to comment.