diff --git a/core/src/com/unciv/models/ruleset/unique/Countables.kt b/core/src/com/unciv/models/ruleset/unique/Countables.kt index f368316a48..302a123d17 100644 --- a/core/src/com/unciv/models/ruleset/unique/Countables.kt +++ b/core/src/com/unciv/models/ruleset/unique/Countables.kt @@ -5,13 +5,15 @@ import com.unciv.models.stats.Stat import com.unciv.models.translations.equalsPlaceholderText import com.unciv.models.translations.getPlaceholderParameters import com.unciv.models.translations.getPlaceholderText +import org.jetbrains.annotations.VisibleForTesting /** - * Prototype for each new [Countables] instance, to ensure a baseline. + * Prototype for each new [Countables] instance, core functionality, to ensure a baseline. * * Notes: * - Each instance ***must*** implement _either_ overload of [matches] and indicate which one via [matchesWithRuleset]. - * - [matches] is used to look up which instance implements a given string, [getErrorSeverity] is responsible for validating placeholders. + * - [matches] is used to look up which instance implements a given string, **without** validating its placeholders. + * - [getErrorSeverity] is responsible for validating placeholders, _and can assume [matches] was successful_. * - Override [getKnownValuesForAutocomplete] only if a sensible number of suggestions is obvious. */ interface ICountable { @@ -30,11 +32,13 @@ interface ICountable { * * Expansion instructions: * - A new simple "variable" needs to implement only [text] and [eval]. + * - Not supplying [text] means the "variable" **must** implement either [matches] overload. If it parses placeholders, then it **must** override [noPlaceholders] to `false`. * - A new "variable" _using placeholder(s)_ needs to implement [matches] and [eval]. - * - Using [simpleName] inside [validate] as the examples do is only done for readability. * - Implement [getErrorSeverity] in most cases, use [UniqueParameterType] to validate each placeholder content. * - Implement [getKnownValuesForAutocomplete] only when a meaningful, not too large set of suggestions is obvious. * - A new countable that draws from an existing enum or set of RulesetObjects should work along the lines of the [Stats] or [TileResources] examples. + * - **Do** heed the docs of [ICountable] - but be aware the [Countables] Enum class pre-implements some of the methods. + * - Run the unit tests! There's one checking implementation conventions. * - When implementing a formula language for Countables, create a new object in a separate file with the actual * implementation, then a new instance here that delegates all its methods to that object. And delete these lines. */ @@ -98,7 +102,7 @@ enum class Countables( return cities.count { it.matchesFilter(filter) } } override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueParameterErrorSeverity? = - UniqueParameterType.CityFilter.getErrorSeverity(parameterText.getPlaceholderParameters().first(), ruleset) + UniqueParameterType.CityFilter.getTranslatedErrorSeverity(parameterText, ruleset) }, FilteredUnits("[mapUnitFilter] Units") { @@ -108,7 +112,7 @@ enum class Countables( return unitManager.getCivUnits().count { it.matchesFilter(filter) } } override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueParameterErrorSeverity? = - UniqueParameterType.MapUnitFilter.getErrorSeverity(parameterText.getPlaceholderParameters().first(), ruleset) + UniqueParameterType.MapUnitFilter.getTranslatedErrorSeverity(parameterText, ruleset) override fun getKnownValuesForAutocomplete(ruleset: Ruleset): Set = (ruleset.unitTypes.keys + ruleset.units.keys).mapTo(mutableSetOf()) { "[$it] Units" } }, @@ -122,7 +126,7 @@ enum class Countables( } } override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueParameterErrorSeverity? = - UniqueParameterType.BuildingFilter.getErrorSeverity(parameterText.getPlaceholderParameters().first(), ruleset) + UniqueParameterType.BuildingFilter.getTranslatedErrorSeverity(parameterText, ruleset) override fun getKnownValuesForAutocomplete(ruleset: Ruleset) = setOf() }, @@ -133,7 +137,7 @@ enum class Countables( return civilizations.count { it.isAlive() && it.matchesFilter(filter) } } override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueParameterErrorSeverity? = - UniqueParameterType.CivFilter.getErrorSeverity(parameterText.getPlaceholderParameters().first(), ruleset) + UniqueParameterType.CivFilter.getTranslatedErrorSeverity(parameterText, ruleset) override fun getKnownValuesForAutocomplete(ruleset: Ruleset) = setOf() }, @@ -143,6 +147,8 @@ enum class Countables( val cities = stateForConditionals.civInfo?.cities ?: return null return cities.sumOf { city -> city.getTiles().count { it.matchesFilter(filter) } } } + override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueParameterErrorSeverity? = + UniqueParameterType.TileFilter.getTranslatedErrorSeverity(parameterText, ruleset) override fun getKnownValuesForAutocomplete(ruleset: Ruleset) = setOf() }, @@ -169,9 +175,11 @@ enum class Countables( } } ; + val placeholderText = text.getPlaceholderText() - - val noPlaceholders = text.isNotEmpty() && !text.contains('[') + + @VisibleForTesting + open val noPlaceholders = !text.contains('[') // Leave these in place only for the really simple cases override fun matches(parameterText: String) = if (noPlaceholders) parameterText == text @@ -181,37 +189,25 @@ enum class Countables( open val documentationHeader get() = "`$text`" + (if (shortDocumentation.isEmpty()) "" else " - $shortDocumentation") - override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueParameterErrorSeverity? { + /** Leave this only for Countables without any parameters - they can rely on [matches] having validated enough */ + override fun getErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueParameterErrorSeverity? = null + + override fun getDeprecationAnnotation(): Deprecated? = declaringJavaClass.getField(name).getAnnotation(Deprecated::class.java) + + protected fun UniqueParameterType.getTranslatedErrorSeverity(parameterText: String, ruleset: Ruleset): UniqueType.UniqueParameterErrorSeverity? { + val severity = getErrorSeverity(parameterText.getPlaceholderParameters().first(), ruleset) return when { - matches(parameterText) -> null + severity != UniqueType.UniqueParameterErrorSeverity.PossibleFilteringUnique -> severity matchesWithRuleset -> UniqueType.UniqueParameterErrorSeverity.RulesetSpecific else -> UniqueType.UniqueParameterErrorSeverity.RulesetInvariant } } - - fun getParameterErrors(parameterText: String, ruleset: Ruleset): List { - if (this.noPlaceholders) return emptyList() - - val errors = mutableListOf() - val placeholderParameters = text.getPlaceholderParameters() - - for ((index, parameter) in placeholderParameters.withIndex()) { - val parameterType = UniqueParameterType.safeValueOf(parameter) - val actualParameterText = placeholderParameters[index] - val severity = parameterType.getErrorSeverity(actualParameterText, ruleset) ?: continue - errors.add("`$text` contains parameter `$actualParameterText` which is invalid for type `${parameterType.name}`") - } - return errors - } - - - override fun getDeprecationAnnotation(): Deprecated? = declaringJavaClass.getField(name).getAnnotation(Deprecated::class.java) companion object { fun getMatching(parameterText: String, ruleset: Ruleset?) = Countables.entries .filter { if (it.matchesWithRuleset) it.matches(parameterText, ruleset!!) else it.matches(parameterText) - } + } fun getCountableAmount(parameterText: String, stateForConditionals: StateForConditionals): Int? { val ruleset = stateForConditionals.gameInfo?.ruleset diff --git a/core/src/com/unciv/models/translations/Translations.kt b/core/src/com/unciv/models/translations/Translations.kt index ded8930cab..b32bf06c65 100644 --- a/core/src/com/unciv/models/translations/Translations.kt +++ b/core/src/com/unciv/models/translations/Translations.kt @@ -501,8 +501,8 @@ fun String.getPlaceholderText(): String { } fun String.equalsPlaceholderText(str: String): Boolean { - if (isEmpty() && str.isEmpty()) return true // Empty strings have no .first() - if (isEmpty() != str.isEmpty()) return false + if (isEmpty()) return str.isEmpty() + if (str.isEmpty()) return false // Empty strings have no .first() if (first() != str.first()) return false // for quick negative return 95% of the time return this.getPlaceholderText() == str } diff --git a/tests/src/com/unciv/uniques/CountableTests.kt b/tests/src/com/unciv/uniques/CountableTests.kt index 2a38956685..1c620035c0 100644 --- a/tests/src/com/unciv/uniques/CountableTests.kt +++ b/tests/src/com/unciv/uniques/CountableTests.kt @@ -3,7 +3,12 @@ package com.unciv.uniques import com.unciv.models.metadata.BaseRuleset import com.unciv.models.ruleset.Ruleset import com.unciv.models.ruleset.RulesetCache -import com.unciv.models.ruleset.unique.* +import com.unciv.models.ruleset.unique.Countables +import com.unciv.models.ruleset.unique.StateForConditionals +import com.unciv.models.ruleset.unique.Unique +import com.unciv.models.ruleset.unique.UniqueParameterType +import com.unciv.models.ruleset.unique.UniqueTriggerActivation +import com.unciv.models.ruleset.validation.RulesetErrorList import com.unciv.models.ruleset.validation.RulesetValidator import com.unciv.models.stats.Stat import com.unciv.models.translations.getPlaceholderParameters @@ -24,13 +29,57 @@ class CountableTests { private var civInfo = game.addCiv() private var city = game.addCity(civInfo, game.tileMap[2,0]) + @Test + fun testCountableConventions() { + fun Class.hasOverrideFor(name: String, vararg args: Class): Boolean { + try { + getDeclaredMethod(name, *args) + } catch (ex: NoSuchMethodException) { + return false + } + return true + } + + var fails = 0 + println("Reflection check of the Countables class:") + for (instance in Countables::class.java.enumConstants) { + val instanceClazz = instance::class.java + + val matchesRulesetOverridden = instanceClazz.hasOverrideFor("matches", String::class.java, Ruleset::class.java) + val matchesPlainOverridden = instanceClazz.hasOverrideFor("matches", String::class.java) + if (instance.matchesWithRuleset && !matchesRulesetOverridden) { + println("`$instance` is marked as working _with_ a `Ruleset` but fails to override `matches(String,Ruleset)`,") + fails++ + } else if (instance.matchesWithRuleset && matchesPlainOverridden) { + println("`$instance` is marked as working _with_ a `Ruleset` but overrides `matches(String)` which is worthless.") + fails++ + } else if (!instance.matchesWithRuleset && matchesRulesetOverridden) { + println("`$instance` is marked as working _without_ a `Ruleset` but overrides `matches(String,Ruleset)` which is worthless.") + fails++ + } + if (instance.text.isEmpty() && !matchesPlainOverridden && !matchesRulesetOverridden) { + println("`$instance` has no `text` but fails to override either `matches` overload.") + fails++ + } + + val getErrOverridden = instanceClazz.hasOverrideFor("getErrorSeverity", String::class.java, Ruleset::class.java) + if (instance.noPlaceholders && getErrOverridden) { + println("`$instance` has no placeholders but overrides `getErrorSeverity` which is likely an error.") + fails++ + } else if (!instance.noPlaceholders && !getErrOverridden) { + println("`$instance` has placeholders that must be treated and therefore **must** override `getErrorSeverity` but does not.") + fails++ + } + } + assertEquals("failure count", 0, fails) + } @Test fun testAllCountableParametersAreUniqueParameterTypes() { for (countable in Countables.entries) { val parameters = countable.text.getPlaceholderParameters() for (parameter in parameters) { - assertNotEquals("Countable ${countable.name} parameter ${parameter} is not a UniqueParameterType", + assertNotEquals("Countable ${countable.name} parameter $parameter is not a UniqueParameterType", UniqueParameterType.safeValueOf(parameter), UniqueParameterType.Unknown) } } @@ -76,7 +125,7 @@ class CountableTests { UniqueTriggerActivation.triggerUnique(Unique("Turn this tile into a [Coast] tile"), civInfo, tile = game.tileMap[-3,0]) UniqueTriggerActivation.triggerUnique(Unique("Turn this tile into a [Coast] tile"), civInfo, tile = game.tileMap[3,0]) - val city2 = game.addCity(civInfo, game.tileMap[-2,0], initialPopulation = 9) + game.addCity(civInfo, game.tileMap[-2,0], initialPopulation = 9) val tests = listOf( "Owned [All] Tiles" to 14, "Owned [worked] Tiles" to 8, @@ -111,39 +160,89 @@ class CountableTests { @Test fun testRulesetValidation() { - val mod = Ruleset().apply { - name = "Testing" - globalUniques.uniques.add("[+42 Faith] ") - globalUniques.uniques.add("[-1 Faith] ") - globalUniques.uniques.add("[+1 Happiness] ") - globalUniques.uniques.add("[+1 Happiness] ") - } - game = TestGame { - RulesetCache[mod.name] = mod - RulesetCache.getComplexRuleset(RulesetCache[BaseRuleset.Civ_V_GnK.fullName]!!, listOf(mod)) - } - val ruleset = game.ruleset + val ruleset = setupModdedGame( + "[+42 Faith] ", + "[-1 Faith] ", + "[+1 Happiness] ", // 1 deprecation + "[+1 Happiness] ", + "[+1 Happiness] ", // 1 not a countable, 1 monkeys + "[+1 Gold] ", // 1 not a countable + "[+1 Food] ", + "[+1 Food] ", // 2 not a countable + "[+1 Food] ", // 3 not a countable + "[+1 Food] ", // 1 not a countable + "[+1 Food] ", + "[+1 Food] ", // 1 not a countable + "[+1 Food] ", // 1 not a countable + "[+1 Food] ", + "[+1 Food] ", // 2 not a countable + "[+1 Food] ", + "[+1 Food] ", // 2 not a countable + "[+1 Food] ", + "[+1 Food] ", + // Attention: `Barbarians` is technically a valid TileFilter because it's a CivFilter. Validation can't know it's meaningless for the OwnedTiles countable. + // `Food` is currently not a valid TileFilter, but might be, and the last is just wrong case for a valid filter and should be flagged. + "[+1 Food] ", // 2 not a countable + "[+1 Food] ", + "[+1 Food] ", // 3 not a countable + ) + ruleset.modOptions.isBaseRuleset = true // To get ruleset-specific validation val errors = RulesetValidator(ruleset).getErrorList() - val countDeprecations = errors.count { - it.text.matches(Regex("Countable.*deprecated.*")) + var fails = 0 + val checks = sequenceOf( + "deprecated" to "Countable.*deprecated.*" to 1, + "monkeys" to ".*Monkeys.*not fit.*countable.*" to 1, + "not a countable" to ".*does not fit parameter type countable.*" to 19, + ) + + println("Countables validation over synthetic test input:") + for ((check, expected) in checks) { + val (label, pattern) = check + val actual = errors.count(pattern) + if (actual == expected) continue + fails++ + println("\tTest '$label' should find $expected errors, found: $actual") } - assertEquals("The test mod should flag one deprecated Countable", 1, countDeprecations) - val countInvalid = errors.count { - it.text.matches(Regex(".*Monkeys.*not fit.*countable.*")) - } - assertEquals("The test mod should flag one invalid Countable", 1, countInvalid) + + assertEquals("failure count", 0, fails) + } + + @Test + fun testForEveryWithInvalidCountable() { + setupModdedGame( + "[+42 Faith] ", + "[+1 Happiness] ", + "[+1 Happiness] ", + ) game.makeHexagonalMap(3) civInfo = game.addCiv() city = game.addCity(civInfo, game.tileMap[2,0]) val cityState = game.addCiv(cityStateType = game.ruleset.cityStateTypes.keys.first()) - val cityStateCity = game.addCity(cityState, game.tileMap[-2,0], true) + game.addCity(cityState, game.tileMap[-2,0], true) civInfo.updateStatsForNextTurn() val happiness = Countables.getCountableAmount("Happiness", StateForConditionals(civInfo, city)) // Base 9, -1 city, -3 population +1 deprecated countable should still work, but the bogus one should not assertEquals("Testing Happiness", 6, happiness) } + + private fun setupModdedGame(vararg uniques: String): Ruleset { + val mod = Ruleset() + mod.name = "Testing" + for (unique in uniques) + mod.globalUniques.uniques.add(unique) + game = TestGame { + RulesetCache[mod.name] = mod + RulesetCache.getComplexRuleset(RulesetCache[BaseRuleset.Civ_V_GnK.fullName]!!, listOf(mod)) + } + return game.ruleset + } + + private fun RulesetErrorList.count(pattern: String): Int { + val regex = Regex(pattern) + return count { it.text.matches(regex) } + } }