Replacement Heuristic #132
Conversation
| @@ -10,6 +10,6 @@ class BazelVersionFileSearch(config: Config) { | |||
| private val fileNames = setOf(".bazelversion", ".bazeliskrc") | |||
|
|
|||
| val bazelVersionFiles: List<BazelFileSearch.BazelFile> by lazy { | |||
| config.path.listDirectoryEntries().filter { fileNames.contains(it.name) }.map { BazelFileSearch.BazelFile(it) } | |||
There was a problem hiding this comment.
Ok, let's cleanup the naming.
Let BazelFileSearch expose:
- BazelFile as interface (just like you did)
- factory method for creating the lazy version, i.e. def createBazelFile(path: Path): BazelFile
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.
| get() = path.readText() | ||
| } | ||
|
|
||
| data class BazelFileTest(override val path: Path, override val content: String) : BazelFile |
There was a problem hiding this comment.
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 TestBazelFile. It would also avoid the problem of being detected as a test class by junit :p
| import org.virtuslab.bazelsteward.core.library.LibraryId | ||
|
|
||
| class VersionHeuristic : Heuristic { | ||
| override val name: String |
There was a problem hiding this comment.
doesn't kotlin allow for override val name: String = "version"?
| updateSuggestion: UpdateLogic.UpdateSuggestion<Lib> | ||
| ): FileUpdateSearch.FileChangeSuggestion? { | ||
| val currentVersion = updateSuggestion.currentLibrary.version.value | ||
| val regex = |
There was a problem hiding this comment.
wait what? If current version is just one string, why are you mapping over it? Doesn't it map over all chars?
In case of version 1.0.0, you are looking for \Q1\E.*\Q.\E.*\Q0\E.*\Q.\E.*\Q0\E.* instead of \Q1.0.0\E
So you could find versions like 1.4.0.0 for example
|
|
||
| class WholeVersionHeuristic : Heuristic { | ||
| override val name: String | ||
| get() = "whole-version" |
There was a problem hiding this comment.
same as above, could getter be avoided?
| import org.virtuslab.bazelsteward.core.common.UpdateLogic | ||
| import org.virtuslab.bazelsteward.core.library.LibraryId | ||
|
|
||
| class WholeVersionHeuristic : Heuristic { |
There was a problem hiding this comment.
I see an issue with naming here.
In our domain we have Library that has LibraryId (group and artifact in case of maven) and version.
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
| val currentVersion = updateSuggestion.currentLibrary.version.value | ||
| val regex = | ||
| 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.
I don't see a logic where it fails in case the version is found more than once (in one or more files).
| inner class SearchBuildFilesTest { | ||
|
|
||
| @Test | ||
| fun `should return right position offset`() { |
There was a problem hiding this comment.
right -> correct, as it might be confusing otherwise.
| import org.virtuslab.bazelsteward.maven.MavenLibraryId | ||
| import kotlin.io.path.Path | ||
|
|
||
| class HeuristicTest { |
There was a problem hiding this comment.
There is no case for version being located in a variable and string interpolation is used to include it.
Also there is no test for when there are 2 libraries with the same version - version heuristic should fail.
| fun `should return null when two libraries have same version`() { | ||
| val versionOnlyHeuristic = VersionOnlyHeuristic() | ||
| val lib = MavenCoordinates( | ||
| MavenLibraryId("", ""), |
There was a problem hiding this comment.
To have a real use case, use actual library names here. It would also good to add test for updating these 2 libraries with standard heuristic (or at least to the general updater tests) to show that these 2 libraries can be updated.
lukaszwawrzyk
removed this pull request from the merge queue due to the queue being cleared
* Change replacement heuristic * Expanded heuristic testes * Fixes after lint * Fixes after buildifier github action failed * Fixes after buildifier github action failed * Fixes after review * Changes after lint * Added test cases for same version of two libraries
Fixes #95