Update CountableObjective to support complex targets#448
Update CountableObjective to support complex targets#448ZoeWithTheE wants to merge 9 commits intogabber235:developfrom
Conversation
Feat: replace int target with range/set string parsing Target now accepts a string like "28-61,63,70.." instead of a single int. Supports inclusive ranges, individual values, open-ended upper (32..) and lower (..10) bounds, and combinations. Display tag strips open-ended syntax for clean rendering (e.g. "32.." shows as "32").
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCountableObjective's target changed from an integer Var to a string-based TargetSpec parsed from token syntax. Completion now checks membership with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt (2)
95-111: Use one snapshot ofcountandtargetper render.Lines 96-111 read both vars twice: once through
isComplete(player)and again for<count>/<target>. For backed or otherwise dynamic vars, that can produce a completed state from one value and render a different one.CachableFactObjective.display()already avoids this by resolving its values once before branching.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt` around lines 95 - 111, Read and cache the dynamic values once at the start of display(player): call count.get(player) and target.get(player) into local vals (and resolve display.get(player) too if needed), then use those snapshots for the completion check and all subsequent replacements instead of calling isComplete(player) (which currently reads count/target again); replace the isComplete(player) call with a local check using matchesTarget(cachedCount.absoluteValue, cachedTarget) and use cached values in replaceTagPlaceholders and displayTarget so the rendered text and completion state are consistent (refer to functions/fields count.get, target.get, isComplete, matchesTarget, display.get, criteria.matches, displayTarget, inactiveObjectiveDisplay, countableObjectiveDisplay, showingObjectiveDisplay).
87-87: Wrap the@Helptext to the repo's 120-column limit.Line 87 is well past the line-length cap; please split the string across lines.
As per coding guidelines, "Use 4 spaces for indentation and wrap lines at 120 characters in Kotlin code and Gradle Kotlin scripts".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt` at line 87, The `@Help` string on CountableObjective (the annotation at line containing `@Help` in CountableObjective.kt) exceeds the 120-column limit; split the long literal into multiple shorter string segments concatenated inside the `@Help`(...) call (e.g. "first chunk " + "second chunk...") or use an equivalent constant string approach so each source line is <=120 chars, preserving the exact help text and punctuation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt`:
- Around line 52-56: The range parsing in CountableObjective (the
trimmed.contains("-") branch) currently splits on every '-' and accepts more
than two pieces, so tokens like "1-2-3" become "1..2"; change the logic in
CountableObjective.kt to reject malformed ranges by ensuring rangeParts.size ==
2 before using rangeParts[0] and rangeParts[1], then parse both with toIntOrNull
and only return true if both are non-null and count in from..to; any other case
should be treated as invalid (return false or propagate the invalid token
behavior used elsewhere).
- Around line 67-71: The displayTarget helper currently trims open-ended syntax
only once on the whole string, which lets tokens like "32..,50" or "..10,28.."
leak; update displayTarget in CountableObjective (displayTarget) to split the
raw string by commas, normalize each token individually by trimming and removing
leading ".." or trailing ".." (same logic as currently applied to the whole
string), and then rejoin the normalized tokens with commas so each target
segment is rendered without open-ended syntax.
---
Nitpick comments:
In
`@extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt`:
- Around line 95-111: Read and cache the dynamic values once at the start of
display(player): call count.get(player) and target.get(player) into local vals
(and resolve display.get(player) too if needed), then use those snapshots for
the completion check and all subsequent replacements instead of calling
isComplete(player) (which currently reads count/target again); replace the
isComplete(player) call with a local check using
matchesTarget(cachedCount.absoluteValue, cachedTarget) and use cached values in
replaceTagPlaceholders and displayTarget so the rendered text and completion
state are consistent (refer to functions/fields count.get, target.get,
isComplete, matchesTarget, display.get, criteria.matches, displayTarget,
inactiveObjectiveDisplay, countableObjectiveDisplay, showingObjectiveDisplay).
- Line 87: The `@Help` string on CountableObjective (the annotation at line
containing `@Help` in CountableObjective.kt) exceeds the 120-column limit; split
the long literal into multiple shorter string segments concatenated inside the
`@Help`(...) call (e.g. "first chunk " + "second chunk...") or use an equivalent
constant string approach so each source line is <=120 chars, preserving the
exact help text and punctuation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d30325a1-ad9c-488f-9397-844952ad5c1d
📒 Files selected for processing (1)
extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt
…st/entries/audience/objectives/CountableObjective.kt Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
fix: address CodeRabbit review comments - Add limit=2 to range split to reject malformed tokens like "1-2-3" - Normalize displayTarget per comma-separated token so combinations like "32..,50" render correctly instead of leaking open-ended syntax - Snapshot count/target/display vars once per display() call to ensure completion state and rendered values are always consistent - Wrap @Help string to 120-column limit - Fix stray brace syntax error in matchesTarget
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt (1)
68-72:⚠️ Potential issue | 🟡 MinorStill normalize each comma-separated target before rendering.
displayTargetstill strips..only once on the full string, so values like32..,50or..10,28..will leak open-ended syntax into<target>.💡 Proposed fix
-private fun displayTarget(raw: String): String = when { - raw.endsWith("..") -> raw.dropLast(2).trim() - raw.startsWith("..") -> raw.drop(2).trim() - else -> raw -} +private fun displayTarget(raw: String): String = raw + .split(",") + .joinToString(",") { part -> + val trimmed = part.trim() + when { + trimmed.endsWith("..") -> trimmed.dropLast(2).trim() + trimmed.startsWith("..") -> trimmed.drop(2).trim() + else -> trimmed + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt` around lines 68 - 72, displayTarget currently strips leading/trailing ".." only once on the whole raw string, allowing open-ended markers to leak when raw is a comma-separated list; update displayTarget to split raw on commas, normalize each segment by trimming and removing any leading ".." or trailing ".." (e.g., dropFirst(2)/dropLast(2) or equivalent) on each piece, then rejoin the normalized segments with commas (preserving spacing) so values like "32..,50" or "..10,28.." render without open-ended syntax.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt`:
- Around line 52-59: In CountableObjective.kt remove the stray closing brace
that prematurely ends the when expression before the else branch: locate the
when handling the "trimmed" variable (the branch handling trimmed.contains("-")
which computes from/to and checks count in from..to) and delete the extra `}` so
the subsequent `else -> trimmed.toIntOrNull() == count` remains inside the same
when block, restoring valid Kotlin syntax for the function/method that evaluates
the objective.
---
Duplicate comments:
In
`@extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt`:
- Around line 68-72: displayTarget currently strips leading/trailing ".." only
once on the whole raw string, allowing open-ended markers to leak when raw is a
comma-separated list; update displayTarget to split raw on commas, normalize
each segment by trimming and removing any leading ".." or trailing ".." (e.g.,
dropFirst(2)/dropLast(2) or equivalent) on each piece, then rejoin the
normalized segments with commas (preserving spacing) so values like "32..,50" or
"..10,28.." render without open-ended syntax.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 69c8ca20-aa30-45b9-986f-1c6a6c4ee27d
📒 Files selected for processing (1)
extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt
|
@gabber235 I do think the matchesTarget function is not 100% related to the CountableObjective and rather should be a function on its own. Idk what you think about it but for the rest I find it okay. Edit: |
gabber235
left a comment
gabber235
left a comment
There was a problem hiding this comment.
I like the idea for this, but I really don't like the execution. First off, there are a bunch of edge cases that are not covered and don't really throw an error or any warning to the user.
Secondly, I think the sanitization should be done much better. Currently, your display target strips out the .., but that makes it really confusing.
I propose you make this a bit more proper with sealed interfaces and subclasses. Use composition to both parse, check if a number is a range, and simplify (like for the , thing: if we have one range which is captured fully within another, we should remove it). So for example, 23..,10..,50 would only become 10.. because all the other ones are captured within it.
You also need a way to represent no valid values, or where all possible values are valid. For example, ..15,10.. allows all values. An empty string `` allows nothing. This way, if we have ,,, we can easily clean it out.
I would make a sealed interface with a companion object containing a parse function which checks how to parse the current thing, then delegates to specific parse functions of the subclasses. Each subclass would have parse, display, contains, and probably a few more functions for the simplification process.
Then we can use it in the display function nicely.
gabber235
force-pushed
the
develop
branch
3 times, most recently
from
4d9cde8 to
1e92538
Compare
|
Thanks for the feedback, rewrote it with a sealed interface. TargetSpec has six impls: ExactTarget, RangeTarget, LowerBoundTarget (N..), UpperBoundTarget (..N), UniversalTarget, and EmptyTarget. Each has contains, subsumes, and display. The parse companion does the comma splitting, delegates to subclass parsers, drops invalid tokens, then simplifies by removing anything sub'd by another spec, so 23..,10..,50 collapses to 10.. as you described. Blank/fully invalid -> EmptyTarget, overlapping ..N and N.. -> UniversalTarget. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt (1)
193-196: Unnecessary null coalescing on non-nullable return types.After the null guard clause at line 191,
playeris smartcast to non-nullPlayer. At lines 194-195,target.get(player)anddisplay.get(player)invoke theVar<T>interface method which returns non-nullableT, so the?: ""operators are unnecessary.Proposed simplification
val currentCount = count.get(player) - val targetSpec = TargetSpec.parse(target.get(player) ?: "") - val displayStr = display.get(player) ?: "" + val targetSpec = TargetSpec.parse(target.get(player)) + val displayStr = display.get(player) val complete = targetSpec.contains(currentCount.absoluteValue)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt` around lines 193 - 196, The code performs unnecessary null-coalescing on values that are already non-null: after the null-guard that smart-casts player to Player, remove the redundant "?: \"\"" on calls to target.get(player) and display.get(player) in CountableObjective (the lines creating targetSpec and displayStr). Keep using TargetSpec.parse(target.get(player)) and display.get(player) directly and leave currentCount and complete logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt`:
- Around line 82-97: The parseToken function treats strings containing "-" as
ranges before attempting exact integer parsing, causing "-5" to be split into a
malformed range and return null; update parseToken so it attempts
token.toIntOrNull() (creating ExactTarget) before the token.contains("-") branch
(or add a guard that treats a leading "-" followed by digits as an exact
integer) so negative integers are parsed into ExactTarget, ensuring
RangeTarget(from,to) still handles proper "a-b" ranges; refer to parseToken,
ExactTarget, RangeTarget, LowerBoundTarget, UpperBoundTarget, and
UniversalTarget when applying the change.
---
Nitpick comments:
In
`@extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt`:
- Around line 193-196: The code performs unnecessary null-coalescing on values
that are already non-null: after the null-guard that smart-casts player to
Player, remove the redundant "?: \"\"" on calls to target.get(player) and
display.get(player) in CountableObjective (the lines creating targetSpec and
displayStr). Keep using TargetSpec.parse(target.get(player)) and
display.get(player) directly and leave currentCount and complete logic
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3cf0d2cb-9571-4159-b197-f5854def181a
📒 Files selected for processing (1)
extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt
Refactor target parsing logic to return null for invalid tokens.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt (1)
121-141:⚠️ Potential issue | 🟡 Minor
<target>still leaks open-ended syntax.
LowerBoundTarget.display()andUpperBoundTarget.display()return32../..10, so Line 208 renders the parser syntax back to players. That reintroduces the same display issue for open-ended targets and composites that include them.💡 Proposed fix
data class LowerBoundTarget(val min: Int) : TargetSpec { @@ - override fun display() = "$min.." + override fun display() = min.toString() } @@ data class UpperBoundTarget(val max: Int) : TargetSpec { @@ - override fun display() = "..$max" + override fun display() = max.toString() }Also applies to: 205-208
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt` around lines 121 - 141, LowerBoundTarget.display() and UpperBoundTarget.display() currently return parser-style open-ended strings like "32.." and "..10", leaking internal syntax to players; change their display implementations to human-friendly phrases (e.g., ">= 32" for LowerBoundTarget.display and "<= 10" for UpperBoundTarget.display) (or use the project’s localization helper if available) so any composite displays that include LowerBoundTarget or UpperBoundTarget no longer show parser syntax; update the display() methods in the LowerBoundTarget and UpperBoundTarget classes accordingly.
🧹 Nitpick comments (1)
extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt (1)
52-79: Extract the parse phases into helpers instead of documenting them inline.
TargetSpec.parse()is doing tokenization, simplification, and universal-coverage detection in one block, and Lines 62-69 rely on inline comments to explain the flow. Pulling those phases into small private helpers would keep the function focused and remove the guideline violation.As per coding guidelines "Keep functions short and focused; prefer guard clauses over nested conditionals" and "Avoid inline comments; refactor unclear code instead of adding explanatory comments".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt` around lines 52 - 79, TargetSpec.parse currently performs tokenization (using parseToken), simplification and universal-coverage detection inline; extract each phase into small private helpers to keep parse short: create e.g. private fun tokenizeAndParse(raw: String): List<TargetSpec> that does the split/map/filter/mapNotNull with parseToken and guard for EmptyTarget, private fun simplifyTokens(tokens: List<TargetSpec>): List<TargetSpec> that implements the subsumption filter (the simplified logic currently using candidate/other), and private fun detectsUniversal(tokens: List<TargetSpec>): Boolean that computes hasLower/hasUpper using UpperBoundTarget and LowerBoundTarget and returns true when the computed lower/upper imply UniversalTarget; then have parse delegate to these helpers and return EmptyTarget, UniversalTarget, a single token, or CompositeTarget as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt`:
- Around line 82-90: The parser in CountableObjective.parseToken allows signed
targets (e.g., "-5", "-5..", "..-5") but the completion check uses
currentCount.absoluteValue, causing mismatches; update the matching logic (the
code that checks targets around where currentCount.absoluteValue is used and the
TargetSpec implementations like ExactTarget, LowerBoundTarget, UpperBoundTarget)
to compare using the signed currentCount (or otherwise normalize both parsed
targets and currentCount the same way) so negative targets are matched correctly
and lower/upper bounds respect the sign produced by parseToken.
- Around line 62-65: The subsumption pass removes duplicate-value tokens because
it compares references (other !== candidate) so two equal tokens are treated as
distinct and subsume each other; fix by deduplicating the tokens list before
running the subsumption logic (e.g., replace the source list with
tokens.distinct() or tokens.distinctBy { /* key */ } and then run the existing
tokens.filter { candidate -> tokens.none { other -> other !== candidate &&
other.subsumes(candidate) } } logic against that deduplicated list); reference
the tokens variable, the filter lambda (candidate/other) and the subsumes(...)
method when making the change.
---
Duplicate comments:
In
`@extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt`:
- Around line 121-141: LowerBoundTarget.display() and UpperBoundTarget.display()
currently return parser-style open-ended strings like "32.." and "..10", leaking
internal syntax to players; change their display implementations to
human-friendly phrases (e.g., ">= 32" for LowerBoundTarget.display and "<= 10"
for UpperBoundTarget.display) (or use the project’s localization helper if
available) so any composite displays that include LowerBoundTarget or
UpperBoundTarget no longer show parser syntax; update the display() methods in
the LowerBoundTarget and UpperBoundTarget classes accordingly.
---
Nitpick comments:
In
`@extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt`:
- Around line 52-79: TargetSpec.parse currently performs tokenization (using
parseToken), simplification and universal-coverage detection inline; extract
each phase into small private helpers to keep parse short: create e.g. private
fun tokenizeAndParse(raw: String): List<TargetSpec> that does the
split/map/filter/mapNotNull with parseToken and guard for EmptyTarget, private
fun simplifyTokens(tokens: List<TargetSpec>): List<TargetSpec> that implements
the subsumption filter (the simplified logic currently using candidate/other),
and private fun detectsUniversal(tokens: List<TargetSpec>): Boolean that
computes hasLower/hasUpper using UpperBoundTarget and LowerBoundTarget and
returns true when the computed lower/upper imply UniversalTarget; then have
parse delegate to these helpers and return EmptyTarget, UniversalTarget, a
single token, or CompositeTarget as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cff43815-2b6c-4bbc-be70-114d305f77ff
📒 Files selected for processing (1)
extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.kt
delegate parsing to subclasses, snippet display, drop absoluteValue
|
Parsing is now fully delegated and each subclass has its own tryParse function, and the central parse iterates a list of parsers. Removed absoluteValue as suggested. Display strings are all snippet-backed with tag placeholders (, , ) so they're translatable, including the composite separator since that varies by language. |
gabber235
left a comment
gabber235
left a comment
There was a problem hiding this comment.
I think this is really taking shape now! There are a few small things that should still be fixed. Additionally, I would recommend writing some tests for the parsing, including golden tests (checking expected outputs), some edge cases, and invalid inputs (where invalid strings are given) to validate correct behaviour.
| fun tryParse(token: String): RangeTarget? { | ||
| if (!token.contains("-")) return null | ||
| val parts = token.split("-", limit = 2) | ||
| val from = parts[0].trim().toIntOrNull() | ||
| val to = parts.getOrNull(1)?.trim()?.toIntOrNull() | ||
| return if (from != null && to != null && from <= to) RangeTarget(from, to) else null | ||
| } |
There was a problem hiding this comment.
Wait is this still correct? Your comment says [min]..[max] but I believe this is checking for [min]-[max].
There was a problem hiding this comment.
Again tests would likely really help with catching stuff like this 😉
There was a problem hiding this comment.
Oh dreaded tests 😅
Where do you want them? In file or another tests location, had a quick poke around and didn't see anything obvious. I'll be very busy over the next few days, so fixes may be slow
There was a problem hiding this comment.
Can you put in the extension testing folder? Make the package the same as the where this is located.
| fun tryParse(token: String): UniversalTarget? = | ||
| if (token == "*") UniversalTarget else null | ||
| } |
There was a problem hiding this comment.
Maybe also make .. parse as universal? As it follows a bit more nicely from the other targets
|
Split TargetSpec out into its own file, the snippet delegates in CountableObjective init through Koin at class load so anything in the same file isn't testable without a full server context. Display is a private extension function in CountableObjective using a when over all the subclasses. Fixed the KDoc on RangeTarget (said [min]..[max], it's [min]-[max]), ".." now parses as UniversalTarget alongside "*", and added distinct() before the subsumption pass so parse("5,5") gives ExactTarget(5) instead of EmptyTarget. Tests are in extensions/QuestExtension/src/test with the same package as requested, covering golden cases, simplification, edge cases, contains and subsumes. |
- Extract TargetSpec sealed interface and subclasses into TargetSpec.kt so
the parser is testable without a Koin/server context
- Display moved to private extension function in CountableObjective.kt
- Fix RangeTarget KDoc ([min]..[max] -> [min]-[max])
- ".." now parses as UniversalTarget alongside "*"
- Add distinct() before subsumption pass to fix parse("5,5") -> EmptyTarget
- Add tests in src/test with same package covering parse, contains, subsumes
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
extensions/QuestExtension/src/test/kotlin/com/typewritermc/quest/entries/audience/objectives/TargetSpecTest.kt (1)
106-135: Consider adding tests for negative integers.The past review discussion addressed negative integer parsing (e.g.,
"-5"as exact,"-5.."as lower bound,"..-5"as upper bound). Adding explicit tests would document and protect this behavior:test("negative exact value") { TargetSpec.parse("-5") shouldBe ExactTarget(-5) } test("negative lower bound") { TargetSpec.parse("-5..") shouldBe LowerBoundTarget(-5) } test("negative upper bound") { TargetSpec.parse("..-5") shouldBe UpperBoundTarget(-5) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/QuestExtension/src/test/kotlin/com/typewritermc/quest/entries/audience/objectives/TargetSpecTest.kt` around lines 106 - 135, Add tests to cover negative integer parsing by extending the "parse invalid and edge inputs" context: add a test asserting TargetSpec.parse("-5") shouldBe ExactTarget(-5), another asserting TargetSpec.parse("-5..") shouldBe LowerBoundTarget(-5), and a third asserting TargetSpec.parse("..-5") shouldBe UpperBoundTarget(-5); place them alongside the existing cases so TargetSpec.parse, ExactTarget, LowerBoundTarget, and UpperBoundTarget behavior for negative values is explicitly verified.extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/TargetSpec.kt (1)
144-148: ClarifyCompositeTarget.subsumessemantics.The current implementation returns
trueif any spec in the composite subsumesother:override fun subsumes(other: TargetSpec) = specs.any { it.subsumes(other) }This is correct for the simplification pass (removing specs subsumed by any member), but semantically a composite subsumes another spec only if the composite's union covers all values of
other. For example,CompositeTarget([ExactTarget(1), ExactTarget(3)])would not subsumeRangeTarget(1,3)even though neither individual spec does.Since this is only used internally for simplification and the current behavior is sufficient for that purpose, this is fine as-is. However, a brief comment clarifying this design choice would help future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/TargetSpec.kt` around lines 144 - 148, Document CompositeTarget.subsumes semantics: add a brief comment on the CompositeTarget data class (and specifically the subsumes override) explaining that subsumes returns true if any contained TargetSpec subsumes the other (i.e., it's a heuristic for the simplification pass) and does not mean the union of specs fully covers the other TargetSpec; reference CompositeTarget, subsumes(other: TargetSpec) and TargetSpec in the comment so future maintainers understand this intentional design choice and its limited use.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/TargetSpec.kt`:
- Around line 82-91: RangeTarget.tryParse currently splits on '-' and fails for
negative ranges; update RangeTarget.tryParse to use a regex like (-?\d+)-(-?\d+)
to capture two signed integers, parse each with toIntOrNull, validate from <=
to, and return RangeTarget(from, to) only on success; also update the `@Help` text
in CountableObjective to reflect that negative ranges (e.g., -5--2) are now
supported (or explicitly state limitation if you choose not to implement
parsing).
---
Nitpick comments:
In
`@extensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/TargetSpec.kt`:
- Around line 144-148: Document CompositeTarget.subsumes semantics: add a brief
comment on the CompositeTarget data class (and specifically the subsumes
override) explaining that subsumes returns true if any contained TargetSpec
subsumes the other (i.e., it's a heuristic for the simplification pass) and does
not mean the union of specs fully covers the other TargetSpec; reference
CompositeTarget, subsumes(other: TargetSpec) and TargetSpec in the comment so
future maintainers understand this intentional design choice and its limited
use.
In
`@extensions/QuestExtension/src/test/kotlin/com/typewritermc/quest/entries/audience/objectives/TargetSpecTest.kt`:
- Around line 106-135: Add tests to cover negative integer parsing by extending
the "parse invalid and edge inputs" context: add a test asserting
TargetSpec.parse("-5") shouldBe ExactTarget(-5), another asserting
TargetSpec.parse("-5..") shouldBe LowerBoundTarget(-5), and a third asserting
TargetSpec.parse("..-5") shouldBe UpperBoundTarget(-5); place them alongside the
existing cases so TargetSpec.parse, ExactTarget, LowerBoundTarget, and
UpperBoundTarget behavior for negative values is explicitly verified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8e497166-e448-49b0-8710-8d643c87b5a0
📒 Files selected for processing (4)
extensions/QuestExtension/build.gradle.ktsextensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/CountableObjective.ktextensions/QuestExtension/src/main/kotlin/com/typewritermc/quest/entries/audience/objectives/TargetSpec.ktextensions/QuestExtension/src/test/kotlin/com/typewritermc/quest/entries/audience/objectives/TargetSpecTest.kt
|
all the concrete objectives (BreakBlock, BreedMob, Fish, KillEntity, PlaceBlock, ShearEntity, SmeltObjective, Travel) use |
Negative ranges like -5--2 silently fail (split on first dash gives an empty left-hand side). Not worth regex complexity for quest objectives where fact values are always non-negative. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Facts can be negative so -10--3 and -5-3 style ranges are valid. Switched to regex (-?\d+)-(-?\d+) to handle them correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
3 participants
Feat: replace int target with range/set string parsing
Target now accepts a string like "28-61,63,70.." instead of a single int. Supports inclusive ranges, individual values, open-ended upper (32..) and lower (..10) bounds, and combinations. Display tag strips open-ended syntax for clean rendering (e.g. "32.." shows as "32").
Summary by CodeRabbit
New Features
Tests
Chores