Refactor `UpdateInfoUrlFinder`, enhance `ForgeType` #3145

rtyley · 2023-08-19T15:17:00Z

This PR has 3 commits, which each can be reviewed separately and contain a single refactor:

➡️ replace conditionals in UpdateInfoUrlFinder with polymorphism on ForgeType - responsibility for generating urls displaying diffs & files moves to ForgeType
📦 introduce ForgeRepo as a parameter object
📦 introduce Version.Update as a parameter object

(as it happens, each subsequent refactor here is smaller than the last)

Cumulatively, the benefits of these changes are:

clearer responsibility: when supporting a new type of forge, the responsibility is now solely implementing a new instance of ForgeType, without also having to search out additional switch statements elsewhere in the codebase
reduced parameter count for several methods - for instance, possibleUpdateInfoUrls() & possibleVersionDiffs() both go from 5 to 2 parameters.
more focused testing of generated urls: a new test suite, ForgeRepoTest, has been introduced which gives clear examples of what diff and file urls look like for different forge types. The urls included in the tests correspond to real live repos as much as possible, so it's possible to click through on them and see that they do indeed correspond to real urls genuinely served by those different forge hosts (note that Azure diff urls have been fixed). The tests here have some overlap with UpdateInfoUrlFinderTest, but are more focussed on making sure that generated url structure is correct, rather than ensuring the correct info sources (release notes, diffs) have been identified for a given dependency update.

This change replaces several switch statements in `UpdateInfoUrlFinder` by adding a couple of additional methods to `ForgeType`. Overall, I feel the code under `possibleUpdateInfoUrls()` is much simplified, and several methods now have one fewer parameter (`forgeUrl`). https://www.refactoring.com/catalog/replaceConditionalWithPolymorphism.html https://refactoring.guru/replace-conditional-with-polymorphism The thing I especially like about this pattern is that as new types of forge are implemented, the responsiblites for supporting that new `ForgeType` are all in one place, rather than spread across several methods elsewhere in the codebase. `ForgeType` gains two new fields of type `FileUriPattern` & `DiffUriPattern`, which are responsible for understanding the url structure of files & diffs in that forge type. `FileUriPattern` & `DiffUriPattern` have been designed so that they can be concisely implemented using the lambda syntax for Single Abstract Method (SAM) types: https://www.scala-lang.org/news/2.12.0/#lambda-syntax-for-sam-types They also can be easily copied as values and used by different forge types that have the same url structure (eg GitLab has exactly the same url structure as GitHub, whereas Gitea has the same url structure for diffs, but not files). Meaning of `ForgeType` parameter changes on some methods ======================================================== Note that with this change, the semantics of the `ForgeType` parameter passed to the `possibleVersionDiffs()` & `possibleUpdateInfoUrls()` methods changes: * OLD meaning : "assume the repo has this forge type, if the repo's host matchs the configured host, otherwise work out the forge type from the repos host". * NEW meaning : "this is the determined forge type of the repo" - already determined as described by the logic described above The method for working out the `ForgeType` of a repo has also moved from `UpdateInfoUrlFinder.forgeTypeFromRepoUrl()` to `ForgeType.fromRepoUrl()` - and it's now called only once, not twice. Deleted tests ------------- Two test cases in `UpdateInfoUrlFinderTest` are deleted because they are testing that `possibleVersionDiffs()` & `possibleUpdateInfoUrls()` would return an empty list if called with a repo url that doesn't have an identifiable `ForgeType` - those methods would now not get called at all, so the tests don't make sense, and can be removed. Azure 'diff' URLs bugfix ======================== The format of Azure 'diff' urls is slightly different to what Scala Steward has been generating up to now - the tag names used in actual Azure urls are prefixed with `GT` on the query parameter values: https://dev.azure.com/rtyley/scala-steward-testing/_git/scala-steward-testing/branchCompare?baseVersion=GTv1.0.0&targetVersion=GTv1.0.1 Removing the `GT` prefixes will give an error: https://dev.azure.com/rtyley/scala-steward-testing/_git/scala-steward-testing/branchCompare?baseVersion=v1.0.0&targetVersion=v1.0.1

`forgeType` & `repoUrl` are two parameters that are passed together in several method calls, suggesting that they belong together in a single parameter object - a `ForgeRepo` where the type of forge indicated by the repoUrl has been established:: https://refactoring.com/catalog/introduceParameterObject.html https://refactoring.guru/introduce-parameter-object https://docondev.com/blog/2020/6/2/refactoring-introduce-parameter-object Those two pieces of information - `forgeType` & `repoUrl` - are all that are required to generate urls for diffs or file display, meaning that actually `ForgeRepo` can be more than a simple parameter object, and provide convenient methods to generate those urls. New tests for generated file & diff urls ======================================== This change also adds `ForgeRepoTest`, which gives clear examples of what diff and file urls should be for different forge types. The urls included in the test correspond to real live repos as much as possible, so it's possible to click through on them and see that they do indeed correspond to real live urls genuinely served by those different forge hosts.

The `currentVersion` & `nextVersion` parameters are paired together relatively often, and because they are of the same type (`Version`) they are susceptible to being accidentally swapped over without any compiler error being raised. Encapsulating them into a single concept (that of a `Version.Update`) makes code a little clearer, and reduces the number of individual parameters being passed on method calls.

codecov · 2023-08-19T15:23:44Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03% 🎉

Comparison is base (77ea181) 91.03% compared to head (905250c) 91.07%.
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3145      +/-   ##
==========================================
+ Coverage   91.03%   91.07%   +0.03%     
==========================================
  Files         162      163       +1     
  Lines        3404     3406       +2     
  Branches      321      307      -14     
==========================================
+ Hits         3099     3102       +3     
+ Misses        305      304       -1

Files Changed	Coverage Δ
...la/org/scalasteward/core/application/Context.scala	`75.64% <100.00%> (+0.31%)`	⬆️
...calasteward/core/coursier/DependencyMetadata.scala	`100.00% <100.00%> (ø)`
...ain/scala/org/scalasteward/core/data/Version.scala	`100.00% <100.00%> (ø)`
.../scala/org/scalasteward/core/forge/ForgeRepo.scala	`100.00% <100.00%> (ø)`
.../scala/org/scalasteward/core/forge/ForgeType.scala	`100.00% <100.00%> (ø)`
...ala/org/scalasteward/core/nurture/NurtureAlg.scala	`20.12% <100.00%> (ø)`
...calasteward/core/nurture/UpdateInfoUrlFinder.scala	`100.00% <100.00%> (+2.22%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rtyley · 2023-08-19T15:20:00Z

modules/core/src/main/scala/org/scalasteward/core/forge/ForgeType.scala

+  trait DiffUriPattern { def forDiff(from: String, to: String): Uri => Uri }
+  trait FileUriPattern { def forFile(fileName: String): Uri => Uri }


FileUriPattern & DiffUriPattern have been designed so that they can be concisely implemented using the lambda syntax for Single Abstract Method (SAM) types, eg:

scala-steward/modules/core/src/main/scala/org/scalasteward/core/forge/ForgeType.scala

Lines 88 to 89 in 13380eb

val diffs: DiffUriPattern = (from, to) => _ / "compare" / s"$from...$to"

val files: FileUriPattern = fileName => _ / "blob" / "master" / fileName

rtyley · 2023-08-19T15:22:24Z

modules/core/src/test/scala/org/scalasteward/core/nurture/UpdateInfoUrlFinderTest.scala

+        s"${onPremForgeUrl}foo/bar/branchCompare?baseVersion=GTv$v1&targetVersion=GTv$v2",
+        s"${onPremForgeUrl}foo/bar/branchCompare?baseVersion=GT$v1&targetVersion=GT$v2",
+        s"${onPremForgeUrl}foo/bar/branchCompare?baseVersion=GTrelease-$v1&targetVersion=GTrelease-$v2"


Note that the format of Azure 'diff' urls is slightly different to what Scala Steward has been generating up to now - the tag names used in actual Azure urls are prefixed with GT on the query parameter values:

https://dev.azure.com/rtyley/scala-steward-testing/_git/scala-steward-testing/branchCompare?baseVersion=GTv1.0.0&targetVersion=GTv1.0.1

Removing the GT prefixes will get a Unrecognized VersionSpec format error from Azure:

https://dev.azure.com/rtyley/scala-steward-testing/_git/scala-steward-testing/branchCompare?baseVersion=v1.0.0&targetVersion=v1.0.1

rtyley · 2023-08-19T15:29:06Z

modules/core/src/test/scala/org/scalasteward/core/nurture/UpdateInfoUrlFinderTest.scala

    assertEquals(
-      possibleVersionDiffs(
-        GitHub,
-        onPremForgeUrl,
-        uri"https://scalacenter.github.io/scalafix/",
-        v1,
-        v2
-      ),
-      List.empty
-    )


This assertion is deleted because it was testing that possibleVersionDiffs() would return an empty list if called with a repo url that doesn't have an identifiable ForgeType - possibleVersionDiffs() is now only ever called if a ForgeType has been identified, and that information is part of the required ForgeRepo parameter on the possibleVersionDiffs() method.

Consequently this test no longer applies for the new method signature/parameter semantic! A similar assertion was deleted for possibleUpdateInfoUrls() below.

Makes sense! Thanks for cleaning that up 😊

rtyley · 2023-08-19T15:30:42Z

modules/core/src/test/scala/org/scalasteward/core/nurture/UpdateInfoUrlFinderTest.scala

-  test("possibleUpdateInfoUrls: repoUrl is a home page") {
-    val repoUrl = uri"https://scalacenter.github.io/scalafix/"
-    val obtained = possibleUpdateInfoUrls(GitHub, uri"https://github.com", repoUrl, v1, v2)
-    assertEquals(obtained, List.empty)
-  }


This testcase is deleted because it was testing that possibleUpdateInfoUrls() would return an empty list if called with a repo url that doesn't have an identifiable ForgeType - possibleUpdateInfoUrls() is now only ever called if a ForgeType has been identified, and that information is part of the required ForgeRepo parameter on the possibleUpdateInfoUrls() method.

Consequently this test no longer applies for the new method signature/parameter semantic! A similar assertion was deleted for possibleVersionDiffs() above.

alejandrohdezma

Thank you very much for this @rtyley. This was one of the best PRs I've ever encounter. Such a pleasure to review!

alejandrohdezma · 2023-08-30T06:07:44Z

modules/core/src/main/scala/org/scalasteward/core/forge/ForgeType.scala

 import org.scalasteward.core.forge.ForgeType._
 import org.scalasteward.core.util.unexpectedString

 sealed trait ForgeType extends Product with Serializable {
  def publicWebHost: Option[String]
+  val diffs: DiffUriPattern


quibble: Can we add some ScalaDocs here to explain what this method should do?

_{This comment follows the conventionalcomments.org standard}

Thanks, done with 905250c !

alejandrohdezma · 2023-08-30T06:08:16Z

modules/core/src/main/scala/org/scalasteward/core/forge/ForgeType.scala

+  trait DiffUriPattern { def forDiff(from: String, to: String): Uri => Uri }
+  trait FileUriPattern { def forFile(fileName: String): Uri => Uri }


alejandrohdezma · 2023-08-30T06:10:20Z

modules/core/src/main/scala/org/scalasteward/core/forge/ForgeType.scala

+      _ / "branchCompare" withQueryParams Map(
+        "baseVersion" -> s"GT$from",
+        "targetVersion" -> s"GT$to"


suggestion: You can also use the operator to add query params +?

_ / "branchCompare" +? ("baseVersion", s"GT$from") +? ("targetVersion", s"GT$to")

_{This comment follows the conventionalcomments.org standard}

Ooh, nice one! Updated with fca39d4 👍

alejandrohdezma · 2023-08-30T06:13:15Z

modules/core/src/test/scala/org/scalasteward/core/nurture/UpdateInfoUrlFinderTest.scala

    assertEquals(
-      possibleVersionDiffs(
-        GitHub,
-        onPremForgeUrl,
-        uri"https://scalacenter.github.io/scalafix/",
-        v1,
-        v2
-      ),
-      List.empty
-    )


Makes sense! Thanks for cleaning that up 😊

alejandrohdezma · 2023-08-30T06:16:17Z

modules/core/src/main/scala/org/scalasteward/core/forge/ForgeRepo.scala

+  * forge, etc, then we can know how to construct many of the urls for common resources existing at
+  * that repo host- for instance, the url to view a particular file, or to diff two commits.
+  */
+case class ForgeRepo(forgeType: ForgeType, repoUrl: Uri) {


praise: 👏🏼 👏🏼 👏🏼

_{This comment follows the conventionalcomments.org standard}

alejandrohdezma · 2023-08-30T06:17:31Z

modules/core/src/test/scala/org/scalasteward/core/forge/ForgeRepoTest.scala

+/** As much as possible, uris in this test suite should aim to be real, clickable, uris that
+  * actually go to real pages, allowing developers working against this test suite to verify that
+  * the url patterns constructed here actually _are_ valid, and match what forges are currently
+  * using in the real world.
+  */


praise: This is gold!

_{This comment follows the conventionalcomments.org standard}

alejandrohdezma · 2023-08-30T06:20:12Z

modules/core/src/main/scala/org/scalasteward/core/data/Version.scala

@@ -124,6 +124,8 @@ final case class Version(value: String) {
 }

 object Version {
+  case class Update(currentVersion: Version, nextVersion: Version)


praise: 👏🏼 👏🏼

_{This comment follows the conventionalcomments.org standard}

@alejandrohdezma

Using the concise `+?` operator suggested by @alejandrohdezma in review: https://github.com/scala-steward-org/scala-steward/pull/3145/files#r1309713985 https://github.com/http4s/http4s/blob/1dead4fc0e4045bc73d5eb08857225634b723c90/core/shared/src/main/scala/org/http4s/QueryOps.scala#L51-L53

@alejandrohdezma

Adding scaladoc as suggested by @alejandrohdezma in review: scala-steward-org#3145 (comment) See also: scala-steward-org#3145 (comment)

rtyley · 2023-08-30T07:36:02Z

Thank you very much for this @rtyley. This was one of the best PRs I've ever encounter. Such a pleasure to review!

Ah what lovely feedback! Thank you! ✨

alejandrohdezma

Thanks for addressing those so quickly! Amazing contribution!

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!

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: #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 #3300 is more urgent!

rtyley added 3 commits August 19, 2023 15:53

rtyley commented Aug 19, 2023

View reviewed changes

alejandrohdezma reviewed Aug 30, 2023

View reviewed changes

rtyley added 2 commits August 30, 2023 08:15

Add Scaladoc on ForgeType's new diffs & files fields

905250c

Adding scaladoc as suggested by @alejandrohdezma in review: scala-steward-org#3145 (comment) See also: scala-steward-org#3145 (comment)

alejandrohdezma approved these changes Aug 30, 2023

View reviewed changes

alejandrohdezma merged commit b0990ac into scala-steward-org:main Aug 30, 2023
6 checks passed

mzuehlke added this to the 0.26.0 milestone Sep 17, 2023

mzuehlke added the refactoring label Sep 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `UpdateInfoUrlFinder`, enhance `ForgeType` #3145

Refactor `UpdateInfoUrlFinder`, enhance `ForgeType` #3145

rtyley commented Aug 19, 2023 •

edited

Loading

codecov bot commented Aug 19, 2023 •

edited

Loading

rtyley Aug 19, 2023

alejandrohdezma Aug 30, 2023

rtyley Aug 19, 2023

rtyley Aug 19, 2023

alejandrohdezma Aug 30, 2023

rtyley Aug 19, 2023 •

edited

Loading

alejandrohdezma left a comment

alejandrohdezma Aug 30, 2023

rtyley Aug 30, 2023

alejandrohdezma Aug 30, 2023

alejandrohdezma Aug 30, 2023

rtyley Aug 30, 2023

alejandrohdezma Aug 30, 2023

alejandrohdezma Aug 30, 2023

alejandrohdezma Aug 30, 2023

alejandrohdezma Aug 30, 2023

rtyley commented Aug 30, 2023

alejandrohdezma left a comment

		trait DiffUriPattern { def forDiff(from: String, to: String): Uri => Uri }
		trait FileUriPattern { def forFile(fileName: String): Uri => Uri }

	val diffs: DiffUriPattern = (from, to) => _ / "compare" / s"$from...$to"
	val files: FileUriPattern = fileName => _ / "blob" / "master" / fileName

Refactor UpdateInfoUrlFinder, enhance ForgeType #3145

Refactor UpdateInfoUrlFinder, enhance ForgeType #3145

Conversation

rtyley commented Aug 19, 2023 • edited Loading

codecov bot commented Aug 19, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtyley Aug 19, 2023 • edited Loading

Choose a reason for hiding this comment

alejandrohdezma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtyley commented Aug 30, 2023

alejandrohdezma left a comment

Choose a reason for hiding this comment

Refactor `UpdateInfoUrlFinder`, enhance `ForgeType` #3145

Refactor `UpdateInfoUrlFinder`, enhance `ForgeType` #3145

rtyley commented Aug 19, 2023 •

edited

Loading

codecov bot commented Aug 19, 2023 •

edited

Loading

rtyley Aug 19, 2023 •

edited

Loading