Configurable lookup of version to bump #159
Conversation
app/src/main/kotlin/org/virtuslab/bazelsteward/app/UpdateSuggestionsMapper.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/org/virtuslab/bazelsteward/app/UpdateSuggestionsMapper.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/org/virtuslab/bazelsteward/app/UpdateSuggestionsMapper.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/org/virtuslab/bazelsteward/app/UpdateSuggestionsMapper.kt
Outdated
Show resolved
Hide resolved
app/src/test/kotlin/org/virtuslab/bazelsteward/app/SearchPatternProviderTest.kt
Outdated
Show resolved
Hide resolved
app/src/test/kotlin/org/virtuslab/bazelsteward/app/SearchPatternProviderTest.kt
Outdated
Show resolved
Hide resolved
repo-config/src/main/kotlin/org/virtuslab/bazelsteward/config/repo/RepoConfig.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| class FilteredByKind<T : DependencyFilter>(private val configs: List<T>, private val libraryId: LibraryId) { | ||
| fun find(predicate: (T) -> Boolean): T? { | ||
| fun forKind(kind: DependencyKind<*>): FilteredByKind<T> { |
There was a problem hiding this comment.
I don't see a reason to have forKind method, nor findAll
| val filter = applier.forKind(kind) | ||
| val patternSearch = filter.findAllNotNull { kind } | ||
| return patternSearch | ||
| .flatMap { it.pathPatterns } |
There was a problem hiding this comment.
I don't think we should concat anything here. It would not allow to override specific search path for a library if anything is defined for kind.
|
|
||
| private fun resolveForKind(kind: DependencyKind<*>): List<PathPattern> { | ||
| val filter = applier.forKind(kind) | ||
| val patternSearch = filter.findAllNotNull { kind } |
There was a problem hiding this comment.
this method takes a getter, it is supposed to look for a SearchPatternConfig that has some property that is not null. You are just passing here some not null value, i.e. filter.findAllNotNull { searchPatternConfig -> kind } which is equivalent to just taking all values, because kind is not null.
app/src/test/kotlin/org/virtuslab/bazelsteward/app/SearchPatternProviderTest.kt
Outdated
Show resolved
Hide resolved
app/src/test/kotlin/org/virtuslab/bazelsteward/app/SearchPatternProviderTest.kt
Outdated
Show resolved
Hide resolved
app/src/test/kotlin/org/virtuslab/bazelsteward/app/UpdateSuggestionsMapperTest.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/org/virtuslab/bazelsteward/app/UpdateSuggestionsMapper.kt
Outdated
Show resolved
Hide resolved
| - regex:.*\/WORKSPACE[.\w]* | ||
| - "**/*.bzl" | ||
| - kinds: |
There was a problem hiding this comment.
such definition would never be picked up, because the preceding catches this kind
core/src/main/kotlin/org/virtuslab/bazelsteward/core/PathPattern.kt
Outdated
Show resolved
Hide resolved
| @@ -28,7 +28,9 @@ data class App( | |||
| val gitHostClient: GitHostClient, | |||
| val appConfig: AppConfig, | |||
| val repoConfig: RepoConfig, | |||
| val updateRulesProvider: UpdateRulesProvider | |||
| val updateRulesProvider: UpdateRulesProvider, | |||
| val searchPatternProvider: SearchPatternProvider, | |||
| fun find(predicate: (T) -> Boolean): T? { | ||
| class FilteredByKind<T : DependencyFilter>( | ||
| private val configs: List<T>, | ||
| private val libraryId: LibraryId?, |
| val filteredConfigs = configs.filter { predicate(it) } | ||
| return filteredConfigs.firstOrNull { it.dependencies.any { f -> f.test(libraryId) } } | ||
| return libraryId?.let { filteredConfigs.firstOrNull { it.dependencies.any { f -> f.test(libraryId) } } } |
There was a problem hiding this comment.
the original implementation seemed fine. libraryId is always defined and filtering by kind already happened in forLibrary method.
|
|
||
| fun resolveForLibrary(library: Library): List<PathPattern> { | ||
| val filter = applier.forLibrary(library) | ||
| return filter.findNotNull { library } |
There was a problem hiding this comment.
again, this code makes no sense. This method accepts a function Config -> Any?. It will filter for any Configs that have some property as not null. If you pass a function like (config) -> ALWAYS_NOT_NULL VALUE, you just take the first element unconditionally, and this is what you do.
I gave you the exact code, why you took it and modified to be wrong?
| fun resolveForLibrary(library: Library): List<PathPattern> { | ||
| val filter = applier.forLibrary(library) | ||
| return filter.findNotNull { library } | ||
| .let { it?.pathPatterns } |
There was a problem hiding this comment.
such code can be replaced with ?.pathPatterns, no need for let. Also this line is not needed. You should find a value with notNull pathPatterns
| import org.virtuslab.bazelsteward.core.common.TextFile | ||
| import org.virtuslab.bazelsteward.core.library.Library | ||
|
|
||
| class UpdateSuggestionsMapper( |
There was a problem hiding this comment.
As I said, it can't be UpdateSuggestionsMapper, because it doesn't even import UpdateSuggestion now. It finds text files for library.
| return RepoConfigParser().parse(javaClass.classLoader.getResource(resourceName)!!.readText()) | ||
| } | ||
|
|
||
| fun loadTextFileFromResources(javaClass: Class<Any>, fileName: String): TextFile { |
There was a problem hiding this comment.
It should be better to use Class<*> everywhere
| @@ -0,0 +1,58 @@ | |||
| package org.virtuslab.bazelsteward.common | |||
There was a problem hiding this comment.
Note that your 2 new files with test utils should not live in the app source directory (src/main). They should be in (src/test). You can place them in subpackage like org.virtuslab.bazelsteward.testing and create a separate bazel target for it. It should be a library target with testonly = True and tests should depend on it.
Added search-pattern to config. Fixes #61