WiP: Improve matching of DataSourceUrl #260
Conversation
jimonthebarn
changed the title
Qodana Community for JVMIt seems all right 👌 No new problems were found according to the checks applied 💡 Qodana analysis was run in the pull request mode: only the changed files were checked View the detailed Qodana reportTo be able to view the detailed Qodana report, you can either:
To get - name: 'Qodana Scan'
uses: JetBrains/qodana-action@v2024.2.5
with:
upload-result: trueContact Qodana teamContact us at qodana-support@jetbrains.com
|
| @@ -1,42 +1,11 @@ | |||
| package com.github.yusufugurozbek.testcontainers.port.updater | |||
|
|
|||
| import com.intellij.database.dataSource.LocalDataSource | |||
|
|
|||
| class DataSourceUrlExtractor { | |||
There was a problem hiding this comment.
@yusufugurozbek
Now that this actually only extracts the DataSourcesUrls - "segments" I'm not too happy with the naming yet. Whats your take on this? Maybe DataSourceComponentExtractor or DataSourceComponentMatcher which is a better hint at what is actually being returned. Yup naming is hard. :)
|
|
||
| fun `test matchMode WITH_TESTCONTAINERS_PARAMETER successfully returns true for matching beforePort and afterPort containing testcontainers=true`() { | ||
| val dataSourceUrlA = DataSourceUrl("beforePort", "dontCareA", "testcontainers=true") | ||
| val dataSourceUrlB = DataSourceUrl("beforePort", "dontCareB", "dontCareC") |
There was a problem hiding this comment.
@yusufugurozbek
The match mode WITH_TESTCONTAINERS_PARAMETER seems a little weird to me. Especially the condition we're currently checking here which essentially requires for the current DataSourceUrl to have "testcontainers=true" within afterPort but it doesn't care whether the new DataSourceUrl also has it set. This follows the current implementation which I did not want to change as I'm oblivious to the intended semantic for the match mode.
There was a problem hiding this comment.
Could you please check this issue before we discuss it further?
| data class DataSourceUrl(val beforePort: String, val port: String, val afterPort: String?, private val urlExtractor: DataSourceUrlExtractor) { | ||
|
|
||
| constructor(beforePort: String, port: String, afterPort: String?) : this(beforePort, port, afterPort, DataSourceUrlExtractor()) | ||
|
|
||
| init { | ||
| setUrlExtractor(urlExtractor) | ||
| } | ||
|
|
||
| companion object { | ||
| private lateinit var extractor: DataSourceUrlExtractor | ||
|
|
||
| private fun setUrlExtractor(extractor: DataSourceUrlExtractor) { | ||
| if (!::extractor.isInitialized) { | ||
| this.extractor = extractor | ||
| } | ||
| } |
There was a problem hiding this comment.
@yusufugurozbek
This seems a little convoluted here. The issue I was trying to solve is that for whatever reason the test classes could not find the constructor anymore if i define it it with a default value for the extractor like such:
data class DataSourceUrl(val beforePort: String, val port: String, val afterPort: String?, private val urlExtractor: DataSourceUrlExtractor = DataSourceUrlExtractor())
Qodana Community for JVMIt seems all right 👌 No new problems were found according to the checks applied 💡 Qodana analysis was run in the pull request mode: only the changed files were checked View the detailed Qodana reportTo be able to view the detailed Qodana report, you can either:
To get - name: 'Qodana Scan'
uses: JetBrains/qodana-action@v2024.3.4
with:
upload-result: trueContact Qodana teamContact us at qodana-support@jetbrains.com
|
yusufugurozbek
force-pushed
the
improve_matching
branch
from
b480a6b to
f5ef42a
Compare
This branch: