-
Notifications
You must be signed in to change notification settings - Fork 6
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
Replacement Heuristic #132
Changes from 5 commits
654fa81
5c4f964
a524118
9e36f2d
daa15b0
8e9ece9
4c5dcb3
2628ea1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,11 +5,18 @@ import java.nio.file.Path | |
import kotlin.io.path.readText | ||
|
||
class BazelFileSearch(config: Config) { | ||
data class BazelFile(val path: Path) { | ||
interface BazelFile { | ||
val path: Path | ||
val content: String | ||
} | ||
|
||
data class BazelFileLazy(override val path: Path) : BazelFile { | ||
override val content: String | ||
get() = path.readText() | ||
} | ||
|
||
data class BazelFileTest(override val path: Path, override val content: String) : BazelFile | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Put this class in tests rather than in sources and use it from tests. Also I'd name it differently. Minimum would be to have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
|
||
private val fileNames = setOf("""BUILD.bazel""", "BUILD", "WORKSPACE") | ||
private val fileSuffix = ".bzl" | ||
|
||
|
@@ -22,5 +29,5 @@ class BazelFileSearch(config: Config) { | |
.toList() | ||
} | ||
|
||
val buildDefinitions: List<BazelFile> by lazy { buildPaths.map { BazelFile(it) } } | ||
val buildDefinitions: List<BazelFile> by lazy { buildPaths.map { BazelFileLazy(it) } } | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
package org.virtuslab.bazelsteward.core.replacement | ||
|
||
import org.virtuslab.bazelsteward.core.common.BazelFileSearch | ||
import org.virtuslab.bazelsteward.core.common.FileUpdateSearch | ||
import org.virtuslab.bazelsteward.core.common.UpdateLogic | ||
import org.virtuslab.bazelsteward.core.library.LibraryId | ||
|
||
interface Heuristic { | ||
val name: String | ||
fun <Lib : LibraryId> apply( | ||
files: List<BazelFileSearch.BazelFile>, | ||
updateSuggestion: UpdateLogic.UpdateSuggestion<Lib> | ||
): FileUpdateSearch.FileChangeSuggestion? | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
package org.virtuslab.bazelsteward.core.replacement | ||
|
||
import org.virtuslab.bazelsteward.core.common.BazelFileSearch | ||
import org.virtuslab.bazelsteward.core.common.FileUpdateSearch | ||
import org.virtuslab.bazelsteward.core.common.UpdateLogic | ||
import org.virtuslab.bazelsteward.core.library.LibraryId | ||
|
||
class VersionHeuristic : Heuristic { | ||
override val name: String | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. doesn't kotlin allow for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
get() = "version" | ||
|
||
override fun <Lib : LibraryId> apply( | ||
files: List<BazelFileSearch.BazelFile>, | ||
updateSuggestion: UpdateLogic.UpdateSuggestion<Lib> | ||
): FileUpdateSearch.FileChangeSuggestion? { | ||
val currentVersion = updateSuggestion.currentLibrary.version.value | ||
val regex = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wait what? If current version is just one string, why are you mapping over it? Doesn't it map over all chars? So you could find versions like 1.4.0.0 for example There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
currentVersion.map { """(${Regex.escape(it.toString())})""" }.reduce { acc, s -> "$acc.*$s" }.let { Regex(it) } | ||
val matchResult = files.firstNotNullOfOrNull { regex.find(it.content)?.to(it.path) } ?: return null | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see a logic where it fails in case the version is found more than once (in one or more files). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
val versionGroup = matchResult.first.groups[0] ?: return null | ||
return FileUpdateSearch.FileChangeSuggestion( | ||
updateSuggestion.currentLibrary, updateSuggestion.suggestedVersion, matchResult.second, versionGroup.range.first | ||
) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
package org.virtuslab.bazelsteward.core.replacement | ||
|
||
import org.virtuslab.bazelsteward.core.common.BazelFileSearch | ||
import org.virtuslab.bazelsteward.core.common.FileUpdateSearch | ||
import org.virtuslab.bazelsteward.core.common.UpdateLogic | ||
import org.virtuslab.bazelsteward.core.library.LibraryId | ||
|
||
class WholeVersionHeuristic : Heuristic { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see an issue with naming here. The basic heuristic looks for version only. But it looks for the whole version. This heuristic however looks for the whole library, i.e. library id and version. Thus version and whole-version doesn't reflect well what happens. Could call it version-only and whole-library There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
override val name: String | ||
get() = "whole-version" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above, could getter be avoided? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
|
||
override fun <Lib : LibraryId> apply( | ||
files: List<BazelFileSearch.BazelFile>, | ||
updateSuggestion: UpdateLogic.UpdateSuggestion<Lib> | ||
): FileUpdateSearch.FileChangeSuggestion? { | ||
val markers = updateSuggestion.currentLibrary.id.associatedStrings() | ||
val currentVersion = updateSuggestion.currentLibrary.version.value | ||
val regex = | ||
(markers + currentVersion).map { """(${Regex.escape(it)})""" }.reduce { acc, s -> "$acc.*$s" }.let { Regex(it) } | ||
val matchResult = files.firstNotNullOfOrNull { regex.find(it.content)?.to(it.path) } ?: return null | ||
val versionGroup = matchResult.first.groups[3] ?: return null | ||
return FileUpdateSearch.FileChangeSuggestion( | ||
updateSuggestion.currentLibrary, updateSuggestion.suggestedVersion, matchResult.second, versionGroup.range.first | ||
) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,251 @@ | ||
package org.virtuslab.bazelsteward.core.replacement | ||
|
||
import io.kotest.matchers.shouldBe | ||
import org.junit.jupiter.api.Nested | ||
import org.junit.jupiter.api.Test | ||
import org.junit.jupiter.api.TestInstance | ||
import org.virtuslab.bazelsteward.core.common.BazelFileSearch | ||
import org.virtuslab.bazelsteward.core.common.FileUpdateSearch | ||
import org.virtuslab.bazelsteward.core.common.UpdateLogic | ||
import org.virtuslab.bazelsteward.core.config.BumpingStrategy | ||
import org.virtuslab.bazelsteward.core.library.SemanticVersion | ||
import org.virtuslab.bazelsteward.core.library.VersioningSchema | ||
import org.virtuslab.bazelsteward.maven.MavenCoordinates | ||
import org.virtuslab.bazelsteward.maven.MavenLibraryId | ||
import kotlin.io.path.Path | ||
|
||
class HeuristicTest { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no case for version being located in a variable and string interpolation is used to include it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
|
||
private fun getBazelFile(fileName: String): BazelFileSearch.BazelFile { | ||
val url = this::class.java.classLoader.getResource(fileName)!! | ||
return BazelFileSearch.BazelFileTest(Path(url.path), url.readText()) | ||
} | ||
|
||
@Nested | ||
@TestInstance(TestInstance.Lifecycle.PER_CLASS) | ||
inner class SearchBuildFilesTest { | ||
|
||
@Test | ||
fun `should return right position offset`() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right -> correct, as it might be confusing otherwise. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
val fileUpdateSearch = FileUpdateSearch() | ||
val lib = MavenCoordinates( | ||
MavenLibraryId("com.7theta", "utilis"), | ||
SemanticVersion(2, 3, 5, "", ""), | ||
VersioningSchema.SemVer, | ||
BumpingStrategy.Default | ||
) | ||
val suggestedVersion = SemanticVersion(2, 4, 0, "", "") | ||
val updateSuggestion = UpdateLogic.UpdateSuggestion(lib, suggestedVersion) | ||
|
||
val files = listOf(getBazelFile("WORKSPACE.bzlignore")) | ||
|
||
val result: List<FileUpdateSearch.FileChangeSuggestion> = | ||
fileUpdateSearch.searchBuildFiles(files, listOf(updateSuggestion)) | ||
result[0].position shouldBe 2377 | ||
} | ||
|
||
@Test | ||
fun `should return right position offset with wrong artifact`() { | ||
val fileUpdateSearch = FileUpdateSearch() | ||
val lib = MavenCoordinates( | ||
MavenLibraryId("com.10theta", "utilis"), | ||
SemanticVersion(2, 3, 5, "", ""), | ||
VersioningSchema.SemVer, | ||
BumpingStrategy.Default | ||
) | ||
val suggestedVersion = SemanticVersion(2, 4, 0, "", "") | ||
val updateSuggestion = UpdateLogic.UpdateSuggestion(lib, suggestedVersion) | ||
|
||
val files = listOf(getBazelFile("WORKSPACE.bzlignore")) | ||
|
||
val result: List<FileUpdateSearch.FileChangeSuggestion> = | ||
fileUpdateSearch.searchBuildFiles(files, listOf(updateSuggestion)) | ||
result[0].position shouldBe 2377 | ||
} | ||
|
||
@Test | ||
fun `should return right position offset without artifact`() { | ||
val fileUpdateSearch = FileUpdateSearch() | ||
val lib = MavenCoordinates( | ||
MavenLibraryId("", ""), | ||
SemanticVersion(2, 3, 5, "", ""), | ||
VersioningSchema.SemVer, | ||
BumpingStrategy.Default | ||
) | ||
val suggestedVersion = SemanticVersion(2, 4, 0, "", "") | ||
val updateSuggestion = UpdateLogic.UpdateSuggestion(lib, suggestedVersion) | ||
|
||
val files = listOf(getBazelFile("WORKSPACE.bzlignore")) | ||
|
||
val result: List<FileUpdateSearch.FileChangeSuggestion> = | ||
fileUpdateSearch.searchBuildFiles(files, listOf(updateSuggestion)) | ||
result[0].position shouldBe 2377 | ||
} | ||
} | ||
|
||
@Nested | ||
@TestInstance(TestInstance.Lifecycle.PER_CLASS) | ||
inner class WholeVersionHeuristicTest { | ||
|
||
@Test | ||
fun `should return right position WholeVersionHeuristic`() { | ||
val wholeVersionHeuristic = WholeVersionHeuristic() | ||
val lib = MavenCoordinates( | ||
MavenLibraryId("com.7theta", "utilis"), | ||
SemanticVersion(2, 3, 5, "", ""), | ||
VersioningSchema.SemVer, | ||
BumpingStrategy.Default | ||
) | ||
val suggestedVersion = SemanticVersion(2, 4, 0, "", "") | ||
val updateSuggestion = UpdateLogic.UpdateSuggestion(lib, suggestedVersion) | ||
|
||
val files = listOf(getBazelFile("WORKSPACE.bzlignore")) | ||
|
||
val result = wholeVersionHeuristic.apply(files, updateSuggestion)!! | ||
result.position shouldBe 2377 | ||
} | ||
|
||
@Test | ||
fun `should return right position WholeVersionHeuristic with wrong artifact`() { | ||
val wholeVersionHeuristic = WholeVersionHeuristic() | ||
val lib = MavenCoordinates( | ||
MavenLibraryId("com.10theta", "utilis"), | ||
SemanticVersion(2, 3, 5, "", ""), | ||
VersioningSchema.SemVer, | ||
BumpingStrategy.Default | ||
) | ||
val suggestedVersion = SemanticVersion(2, 4, 0, "", "") | ||
val updateSuggestion = UpdateLogic.UpdateSuggestion(lib, suggestedVersion) | ||
|
||
val files = listOf(getBazelFile("WORKSPACE.bzlignore")) | ||
|
||
val result = wholeVersionHeuristic.apply(files, updateSuggestion) | ||
result shouldBe null | ||
} | ||
|
||
@Test | ||
fun `should return right position WholeVersionHeuristic without artifact`() { | ||
val wholeVersionHeuristic = WholeVersionHeuristic() | ||
val lib = MavenCoordinates( | ||
MavenLibraryId("", ""), | ||
SemanticVersion(2, 3, 5, "", ""), | ||
VersioningSchema.SemVer, | ||
BumpingStrategy.Default | ||
) | ||
val suggestedVersion = SemanticVersion(2, 4, 0, "", "") | ||
val updateSuggestion = UpdateLogic.UpdateSuggestion(lib, suggestedVersion) | ||
|
||
val files = listOf(getBazelFile("WORKSPACE.bzlignore")) | ||
|
||
val result = wholeVersionHeuristic.apply(files, updateSuggestion)!! | ||
result.position shouldBe 2377 | ||
} | ||
} | ||
|
||
@Nested | ||
@TestInstance(TestInstance.Lifecycle.PER_CLASS) | ||
inner class VersionHeuristicTest { | ||
|
||
@Test | ||
fun `should return right position offset VersionHeuristic`() { | ||
val versionHeuristic = VersionHeuristic() | ||
val lib = MavenCoordinates( | ||
MavenLibraryId("com.7theta", "utilis"), | ||
SemanticVersion(2, 3, 5, "", ""), | ||
VersioningSchema.SemVer, | ||
BumpingStrategy.Default | ||
) | ||
val suggestedVersion = SemanticVersion(2, 4, 0, "", "") | ||
val updateSuggestion = UpdateLogic.UpdateSuggestion(lib, suggestedVersion) | ||
|
||
val files = listOf(getBazelFile("WORKSPACE.bzlignore")) | ||
|
||
val result = versionHeuristic.apply(files, updateSuggestion)!! | ||
result.position shouldBe 2377 | ||
} | ||
|
||
@Test | ||
fun `should return right position offset VersionHeuristic with wrong artifact`() { | ||
val versionHeuristic = VersionHeuristic() | ||
val lib = MavenCoordinates( | ||
MavenLibraryId("com.10theta", "utilis"), | ||
SemanticVersion(2, 3, 5, "", ""), | ||
VersioningSchema.SemVer, | ||
BumpingStrategy.Default | ||
) | ||
val suggestedVersion = SemanticVersion(2, 4, 0, "", "") | ||
val updateSuggestion = UpdateLogic.UpdateSuggestion(lib, suggestedVersion) | ||
|
||
val files = listOf(getBazelFile("WORKSPACE.bzlignore")) | ||
|
||
val result = versionHeuristic.apply(files, updateSuggestion)!! | ||
result.position shouldBe 2377 | ||
} | ||
|
||
@Test | ||
fun `should return right position offset VersionHeuristic without artifact`() { | ||
val versionHeuristic = VersionHeuristic() | ||
val lib = MavenCoordinates( | ||
MavenLibraryId("", ""), | ||
SemanticVersion(2, 3, 5, "", ""), | ||
VersioningSchema.SemVer, | ||
BumpingStrategy.Default | ||
) | ||
val suggestedVersion = SemanticVersion(2, 4, 0, "", "") | ||
val updateSuggestion = UpdateLogic.UpdateSuggestion(lib, suggestedVersion) | ||
|
||
val files = listOf(getBazelFile("WORKSPACE.bzlignore")) | ||
|
||
val result = versionHeuristic.apply(files, updateSuggestion)!! | ||
result.position shouldBe 2377 | ||
} | ||
} | ||
|
||
@Nested | ||
@TestInstance(TestInstance.Lifecycle.PER_CLASS) | ||
inner class CompareHeuristicTest { | ||
|
||
@Test | ||
fun `should return same position offset for WholeVersionHeuristic and VersionHeuristic`() { | ||
val lib = MavenCoordinates( | ||
MavenLibraryId("com.7theta", "utilis"), | ||
SemanticVersion(2, 3, 5, "", ""), | ||
VersioningSchema.SemVer, | ||
BumpingStrategy.Default | ||
) | ||
val suggestedVersion = SemanticVersion(2, 4, 0, "", "") | ||
val updateSuggestion = UpdateLogic.UpdateSuggestion(lib, suggestedVersion) | ||
|
||
val files = listOf(getBazelFile("WORKSPACE.bzlignore")) | ||
|
||
val wholeVersionHeuristic = WholeVersionHeuristic() | ||
val result1 = wholeVersionHeuristic.apply(files, updateSuggestion)!! | ||
val versionHeuristic = VersionHeuristic() | ||
val result2 = versionHeuristic.apply(files, updateSuggestion)!! | ||
|
||
result1.position shouldBe result2.position | ||
} | ||
|
||
@Test | ||
fun `should return different position offset for WholeVersionHeuristic and VersionHeuristic`() { | ||
val lib = MavenCoordinates( | ||
MavenLibraryId("com.10theta", "utilis"), | ||
SemanticVersion(2, 3, 5, "", ""), | ||
VersioningSchema.SemVer, | ||
BumpingStrategy.Default | ||
) | ||
val suggestedVersion = SemanticVersion(2, 4, 0, "", "") | ||
val updateSuggestion = UpdateLogic.UpdateSuggestion(lib, suggestedVersion) | ||
|
||
val files = listOf(getBazelFile("WORKSPACE.bzlignore")) | ||
|
||
val wholeVersionHeuristic = WholeVersionHeuristic() | ||
val result1 = wholeVersionHeuristic.apply(files, updateSuggestion) | ||
val versionHeuristic = VersionHeuristic() | ||
val result2 = versionHeuristic.apply(files, updateSuggestion)!! | ||
|
||
result1 shouldBe null | ||
result2.position shouldBe 2377 | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,8 @@ | ||
filegroup( | ||
name = "resources", | ||
srcs = glob([".*.yaml"]), | ||
srcs = glob([ | ||
".*.yaml", | ||
"*.bzlignore", | ||
]), | ||
visibility = ["//visibility:public"], | ||
) |
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.
Ok, let's cleanup the naming.
Let BazelFileSearch expose:
So make BazelFileLazy private, just return it from the factory method.
Also rename BazelFileLazy to LazyBazelFile.
It is still not a perfect design, all of this needs to be eventually renamed to something more generic than BazelFile, but let's at least keep what we have now in some order.
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.
Fixed