ignoreLines feature for filtering the input file #307
Conversation
###What's done: * Added ignoreLines option for Fix plugin. * Added tests for ignoreLines and Fix plugin. * Added ignoreLines option for Warn plugin. * Added tests for ignoreLines and Warn plugin. * Implemented copying a file in Warn plugin in order to filter out lines that match any regex from ignoreLines. (#303)
###What's done: * Added ignoreLines option for Warn plugin. * Added tests for ignoreLines and Warn plugin. * Added ignoreLines option for Fix plugin. * Added tests for ignoreLines and Fix plugin. (#303)
| suiteName = "Autofix: Smoke Tests" | ||
|
|
||
| [fix] | ||
| ignoreLines = ["// IGNORE_ME"] |
There was a problem hiding this comment.
and better to test with several different regexes
###What's done: * Added ignoreLines option for Warn plugin. * Added tests for ignoreLines and Warn plugin. * Added ignoreLines option for Fix plugin. * Added tests for ignoreLines and Fix plugin. (#303)
###What's done: * Added ignoreLines option for Warn plugin. * Added tests for ignoreLines and Warn plugin. * Added ignoreLines option for Fix plugin. * Added tests for ignoreLines and Fix plugin. * Implemented copying a file in Warn plugin in order to filter out lines that match any regex from ignoreLines. (#133)
| @@ -74,6 +79,7 @@ data class GeneralConfig( | |||
| val runConfigPattern: Regex? = null, | |||
| ) : PluginConfig { | |||
| override val type = TestConfigSections.GENERAL | |||
| override var ignoreLinesPatterns: MutableList<Regex> = mutableListOf() | |||
There was a problem hiding this comment.
val should be here
| /** | ||
| * list of regexes to be ignored | ||
| */ | ||
| var ignoreLinesPatterns: MutableList<Regex> |
There was a problem hiding this comment.
why var should be here? Because of the hierarchical inheritance of configs?
There was a problem hiding this comment.
val should be here
| @@ -26,14 +26,19 @@ data class FixAndWarnPluginConfig( | |||
| override var configLocation: Path = "undefined_toml_location".toPath() | |||
| override val resourceNamePatternStr: String = "(${fix.resourceNamePatternStr})|(${warn.resourceNamePatternStr})" | |||
|
|
|||
| @Transient | |||
There was a problem hiding this comment.
why @Transient? I though that we deserialize this value from toml config
There was a problem hiding this comment.
The point it that we deserialize ignoreLines from toml config. ignoreLinesPatterns is initialized with ignoreLines that we parse from config so it is not deserialized directly from toml.
| } | ||
| } | ||
| companion object { | ||
| internal val defaultIgnoreLines: MutableList<Regex> = mutableListOf() |
There was a problem hiding this comment.
Do we really need an extra constant for it? May be you will simply set the default value (mutableListOf()) in place, where it is created?
There was a problem hiding this comment.
To my mind it looked nicer. But ok, it seems that we don't need it here.
| val pathCopy: Path = constructPathForCopyOfTestFile(FixPlugin::class.simpleName!!, path) | ||
| createTempDir(pathCopy.parent!!) | ||
|
|
||
| val expectedWarningPattern = generalConfig.expectedWarningsPattern | ||
| val defaultIgnoreLinesPatterns: MutableList<Regex> = mutableListOf() |
There was a problem hiding this comment.
why is it named as defaultIgnoreLinesPatterns? Why "default"? You are adding values to this list, is it really default?
There was a problem hiding this comment.
If ignoreLines is not set in config file, I should use expectedWarningsPattern and runConfigPattern by default. So the values I add to this list are values for default case. In this way I think default is ok here.
| fixPluginConfig.ignoreLinesPatterns.none { pattern -> pattern.matches(line) } | ||
| } | ||
| ?: run { | ||
| defaultIgnoreLinesPatterns.none {regex -> regex.matches(line) } |
There was a problem hiding this comment.
diktat is not working? where are spaces after {
There was a problem hiding this comment.
Interesting. Seems like this case is ignored by diktat.
| fs.readLines(path) | ||
| .filter { line -> | ||
| fixPluginConfig.ignoreLines?.let { | ||
| fixPluginConfig.ignoreLinesPatterns.none { pattern -> pattern.matches(line) } |
There was a problem hiding this comment.
use it in such small lambdas, it looks better with such syntax sugar
| if (expectedWarningPattern == null || !generalConfig.expectedWarningsPattern!!.matches(it)) { | ||
| fs.readLines(path) | ||
| .filter { line -> | ||
| fixPluginConfig.ignoreLines?.let { |
There was a problem hiding this comment.
what will happen here if ignoreLines == null? Sorry, if I am not completely understand this logic
Also I suppose that fixPluginConfig.ignoreLines is a not nullable constant due to your logic and you can move it to some val with !! OUTside of this loop.
There was a problem hiding this comment.
I have two variables: ignoreLines and ignoreLinesPatterns. We deserialize ignoreLines from toml config so it is nullable. Than we use ignoreLinesPatterns is a mutable list of Regex and it is not nullable.
In fix plugin there are three cases: ignoreLines is empty, ignoreLines is null or ignoreLines is a list of regular expressions. Here I check if ignoreLines is null
|
|
||
| fs.write(fs.createFile(pathCopy)) { | ||
| fs.readLines(path).forEach { | ||
| if (expectedWarningPattern == null || !generalConfig.expectedWarningsPattern!!.matches(it)) { |
There was a problem hiding this comment.
warningsPattern will be now included into the ignoreLinesPatterns?
There was a problem hiding this comment.
User should explicitly put warningPattern into ignoreLines in toml config.
| ).also { it.configLocation = this.configLocation } | ||
| other.ignoreLines?.let { | ||
| this.ignoreLines?.let { other.ignoreLines.union(this.ignoreLines) } ?: other.ignoreLines | ||
| }?.toMutableList() ?: this.ignoreLines |
There was a problem hiding this comment.
why do you need to convert it to mutableList? I thought that you created this field as a mutableList from a very beginning.
There was a problem hiding this comment.
The point is that we use .union(), which returns Set.
| } | ||
|
|
||
| // due to probable bug in ktoml, ignoreLines = [] and no ignoreLines is ktoml are parsed to be mutableListOf("null") |
| it.configLocation = this.configLocation | ||
| } | ||
| companion object { | ||
| internal val defaultIgnoreLines: MutableList<Regex> = mutableListOf() |
###What's done: * Added ignoreLines option for Warn plugin. * Added tests for ignoreLines and Warn plugin. * Added ignoreLines option for Fix plugin. * Added tests for ignoreLines and Fix plugin. * Implemented copying a file in Warn plugin in order to filter out lines that match any regex from ignoreLines. (#133)
###What's done: * Added ignoreLines option for Warn plugin. * Added tests for ignoreLines and Warn plugin. * Added ignoreLines option for Fix plugin. * Added tests for ignoreLines and Fix plugin. * Implemented copying a file in Warn plugin in order to filter out lines that match any regex from ignoreLines. (#133)
###What's done: * Added ignoreLines option for Warn plugin. * Added tests for ignoreLines and Warn plugin. * Added ignoreLines option for Fix plugin. * Added tests for ignoreLines and Fix plugin. * Implemented copying a file in Warn plugin in order to filter out lines that match any regex from ignoreLines. (#133)
|
ok, let's test it |
###What's done: * Added ignoreLines option for Warn plugin. * Added tests for ignoreLines and Warn plugin. * Added ignoreLines option for Fix plugin. * Added tests for ignoreLines and Fix plugin. * Implemented copying a file in Warn plugin in order to filter out lines that match any regex from ignoreLines. (#133)
###What's done: * Added ignoreLines option for Warn plugin. * Added tests for ignoreLines and Warn plugin. * Added ignoreLines option for Fix plugin. * Added tests for ignoreLines and Fix plugin. * Implemented copying a file in Warn plugin in order to filter out lines that match any regex from ignoreLines. (#133)
###What's done: * Added ignoreLines option for Warn plugin. * Added tests for ignoreLines and Warn plugin. * Added ignoreLines option for Fix plugin. * Added tests for ignoreLines and Fix plugin. * Implemented copying a file in Warn plugin in order to filter out lines that match any regex from ignoreLines. (#133)
###What's done: * TESTING (#133)
###What's done: * TESTING (#133)
###What's done: * Added ignoreLines option for Warn plugin. * Added tests for ignoreLines and Warn plugin. * Added ignoreLines option for Fix plugin. * Added tests for ignoreLines and Fix plugin. * Implemented copying a file in Warn plugin in order to filter out lines that match any regex from ignoreLines.
###What's done: * Added ignoreLines option for Warn plugin. * Added tests for ignoreLines and Warn plugin. * Added ignoreLines option for Fix plugin. * Added tests for ignoreLines and Fix plugin. * Implemented copying a file in Warn plugin in order to filter out lines that match any regex from ignoreLines.
feature/ignoreLines
This pull request closes #133
What's done: