AI Parsing Fix#63
Conversation
📝 WalkthroughWalkthroughThis PR refactors the AI parser to convert untyped Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Maple2.File.Tests/AiParserTest.cs (1)
64-65: ⚡ Quick winAssert the new section-level fields too.
This update only verifies
Entries, so a regression instartAni,endAni,isBattle, oronlyDeadwould still pass. It also treats attribute-only<battle>/<battleEnd>sections as absent becausehasBattleandhasBattleEnddepend solely onEntries.Count. Please add at least one fixture-specific assertion for the new container fields.Also applies to: 84-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.File.Tests/AiParserTest.cs` around lines 64 - 65, The test currently only checks Entries.Count via hasBattle/hasBattleEnd so add assertions that the section objects exist even when Entries is empty and that their container-level fields (startAni, endAni, isBattle, onlyDead) have expected fixture values; specifically, for data.Battle and data.BattleEnd assert the section itself is not null (to handle attribute-only sections) and assert each of the fields startAni, endAni, isBattle, onlyDead match the fixture values used in the test; apply the same additional assertions in the other test block referenced around the 84-90 region.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Maple2.File.Parser/Maple2.File.Parser.csproj`:
- Line 16: The package version update is too small for the breaking change:
either restore the previous public types for NpcAi.Battle and NpcAi.BattleEnd
(keep List<Entry> shape and add the new container types as new APIs or provide
adapter properties/methods) OR, if you intend to keep the breaking change, bump
the PackageVersion to a new major (e.g., X.0.0) in the project file so the
release is semantically major; modify Maple2.File.Parser.csproj PackageVersion
accordingly and ensure NpcAi.Battle and NpcAi.BattleEnd are updated consistently
with your chosen approach.
---
Nitpick comments:
In `@Maple2.File.Tests/AiParserTest.cs`:
- Around line 64-65: The test currently only checks Entries.Count via
hasBattle/hasBattleEnd so add assertions that the section objects exist even
when Entries is empty and that their container-level fields (startAni, endAni,
isBattle, onlyDead) have expected fixture values; specifically, for data.Battle
and data.BattleEnd assert the section itself is not null (to handle
attribute-only sections) and assert each of the fields startAni, endAni,
isBattle, onlyDead match the fixture values used in the test; apply the same
additional assertions in the other test block referenced around the 84-90
region.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: aedf3b81-cbd5-453f-a514-d5c92d36b118
📒 Files selected for processing (4)
Maple2.File.Parser/AiParser.csMaple2.File.Parser/Maple2.File.Parser.csprojMaple2.File.Parser/Xml/AI/NpcAi.csMaple2.File.Tests/AiParserTest.cs
| <!-- Use following lines to write the generated files to disk. --> | ||
| <EmitCompilerGeneratedFiles Condition=" '$(Configuration)' == 'Debug' ">true</EmitCompilerGeneratedFiles> | ||
| <PackageVersion>2.4.5</PackageVersion> | ||
| <PackageVersion>2.4.6</PackageVersion> |
There was a problem hiding this comment.
Patch bump is too small for this public API break.
NpcAi.Battle and NpcAi.BattleEnd no longer expose List<Entry>; they now expose new public container types. That is a breaking change for downstream callers, so publishing it as 2.4.6 hides incompatible API changes behind a patch update. Please either ship this as a major version or preserve the old shape for a deprecation cycle.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.File.Parser/Maple2.File.Parser.csproj` at line 16, The package version
update is too small for the breaking change: either restore the previous public
types for NpcAi.Battle and NpcAi.BattleEnd (keep List<Entry> shape and add the
new container types as new APIs or provide adapter properties/methods) OR, if
you intend to keep the breaking change, bump the PackageVersion to a new major
(e.g., X.0.0) in the project file so the release is semantically major; modify
Maple2.File.Parser.csproj PackageVersion accordingly and ensure NpcAi.Battle and
NpcAi.BattleEnd are updated consistently with your chosen approach.
Summary by CodeRabbit
Release Notes - Version 2.4.6
Improvements
Chores