Conversation
WalkthroughThis update introduces extensive support for Korean ("KR") locale data parsing across the codebase. It adds new XML-serializable classes, parsing methods, and test cases to handle KR-specific data structures for items, maps, NPCs, quests, skills, riding, scripts, tables, and item options. Many parsers now conditionally select data sources and deserialization types based on locale. Test coverage is expanded with new tests for KR data, including a comprehensive test suite for the Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant Filter
participant Parser (generic)
participant KRParser
participant DataSource (NA)
participant DataSource (KR)
participant XMLClass (NA)
participant XMLClass (KR)
TestRunner->>Filter: Load(region, environment)
alt region == "KR"
TestRunner->>KRParser: new KRParser(TestUtilsKr.XmlReader)
KRParser->>DataSource (KR): Read KR XML data
KRParser->>XMLClass (KR): Deserialize to KR-specific class
KRParser-->>TestRunner: Yield parsed KR data
else
TestRunner->>Parser: new Parser(TestUtils.XmlReader)
Parser->>DataSource (NA): Read NA XML data
Parser->>XMLClass (NA): Deserialize to NA-specific class
Parser-->>TestRunner: Yield parsed NA data
end
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🔭 Outside diff range comments (4)
Maple2.File.Tests/NifParserTest.cs (1)
33-110:⚠️ Potential issue
ValidateNifMeshDataexits silently when no mesh is found
Inside the per‑document loop, youcontinuewhenmeshFoundisfalse(lines 106‑108).
That means a document that should contain PhysX data could silently pass the test.
Fail fast instead—this is a unit test, not production code.-if (!meshFound) { - continue; -} +Assert.IsTrue(meshFound, $"No PhysX mesh was found in {relpath}.");This keeps accidental regressions from slipping through.
Maple2.File.Parser/SkillParser.cs (1)
53-66:⚠️ Potential issueDictionary population can throw on duplicate Skill IDs
skillNames.Add(...)insideLoadSkillNames()will raise an exception if the same ID appears in multipleskillnamefiles.-for each (Key key in mapping.key) { - skillNames.Add(int.Parse(key.id), key.name); -} +foreach (Key key in mapping.key) { + skillNames[int.Parse(key.id)] = key.name; // last one wins +}This makes the parser resilient to duplicate definitions while keeping O(1) lookup.
Maple2.File.Parser/Xml/Table/JobTable.cs (1)
1-2:⚠️ Potential issueMissing
using System/System.Collections.Generic
IList,List,Array, etc. are unresolved without these imports.using System.Xml.Serialization; +using System; +using System.Collections.Generic; using M2dXmlGenerator;Maple2.File.Parser/Xml/Table/IndividualItemDrop.cs (1)
1-2:⚠️ Potential issueCore namespaces missing
Same as previous file; add:
using System.Xml.Serialization; +using System; +using System.Collections.Generic; using M2dXmlGenerator;
🧹 Nitpick comments (24)
Maple2.File.Parser/Xml/Item/Property.cs (1)
45-45: Type change from int to bool improves semanticsThe change from
inttoboolfor themoveDisableproperty better represents its binary nature (either movement is disabled or not). This aligns with the PR's goal of refining XML schema adaptations for Korean locale support.However, ensure this change is backward compatible with existing XML data. If any code was previously checking for specific integer values beyond 0/1, those areas may need updates.
Consider documenting this property with XML comments to clarify its purpose and behavior, especially since it's part of a locale-specific adaptation.
Maple2.File.Tests/TestUtils.cs (2)
54-68: Well-organized KR testing utilities.The implementation of
TestUtilsKRfollows the same pattern asTestUtils, providing consistent access to Korean data files. The error message for missing environment variable is clear and helpful.Consider extracting common code between
TestUtilsandTestUtilsKRto reduce duplication. A base class or shared utility method could be used to initialize readers with different root paths.-public static class TestUtils { +public abstract static class BaseTestUtils { public static readonly M2dReader XmlReader; public static readonly M2dReader ServerReader; // ... other shared fields + protected static void Initialize(string envVarName) { + DotEnv.Load(); + string? m2dPath = Environment.GetEnvironmentVariable(envVarName); + if (string.IsNullOrEmpty(m2dPath)) { + throw new Exception($"{envVarName} is not set"); + } + + // Initialize readers with m2dPath + } } +public static class TestUtils : BaseTestUtils { + static TestUtils() { + Initialize("MS2_DATA_FOLDER"); + } } +public static class TestUtilsKR : BaseTestUtils { + static TestUtilsKR() { + Initialize("KMS2_DATA_FOLDER"); + } }
89-96: Duplicated debug handlers.The debug handlers are identical to those in
TestUtils, which reinforces the suggestion to extract common code.The duplicate debug handlers could be moved to a shared utility class or the suggested base class to avoid repetition.
Maple2.File.Tests/XBlockParserTest.cs (1)
12-19: Consider keeping test methods active even if they're not regularly runInstead of commenting out
TestConversion, consider using the[Ignore]attribute as done withTestXBlockParserAll(line 55). This keeps the test in the codebase without running it, maintaining its visibility while documenting its purpose.- // [TestMethod] - // public void TestConversion() { - // var flatConverter = new FlatToModel(TestUtils.ExportedReader, TestUtils.AssetMetadataReader); - // flatConverter.Convert(); - - // var xblockConverter = new XBlockToBlock(TestUtils.ExportedReader); - // xblockConverter.Convert(); - // } + [Ignore("Conversion test is slow and only needed occasionally")] + [TestMethod] + public void TestConversion() { + var flatConverter = new FlatToModel(TestUtils.ExportedReader, TestUtils.AssetMetadataReader); + flatConverter.Convert(); + + var xblockConverter = new XBlockToBlock(TestUtils.ExportedReader); + xblockConverter.Convert(); + }Maple2.File.Tests/FunctionCubeParserTest.cs (1)
27-39: Well-implemented KR-specific testThe new test method correctly tests the Korean locale version of the function cube parser, following the same pattern as the NA test and validating the expected count of 433 entries.
Consider adding the commented-out event handler subscriptions for unknown XML elements and attributes as in the NA test for consistency:
var parser = new FunctionCubeParser(TestUtilsKR.XmlReader); + // parser.FunctionCubeSerializer.UnknownElement += TestUtils.UnknownElementHandler; + // parser.FunctionCubeSerializer.UnknownAttribute += TestUtils.UnknownAttributeHandler; int count = 0;Maple2.File.Tests/TableParserTest.cs (1)
471-472: Remove redundant assignment.The results are assigned to a variable but never used. The following line re-parses the same data.
- var results = _parser.ParseAdventureLevelAbility(); - foreach ((_, _) in _parser.ParseAdventureLevelAbility()) {Maple2.File.Parser/ItemParser.cs (1)
23-47: Generic Parse method to handle multiple item data structures.The
Parse<T>()method effectively handles both regular and Korean-specific item data structures through:
- Type parameter
Tconstrained to class types- Conditional folder path selection based on locale
- Type-specific processing in the switch statement
This is a clean approach that avoids code duplication while maintaining type safety.
However, the method could potentially throw an exception if an unexpected type is passed. Consider adding a default case to handle unexpected types:
switch (xml) { case ItemDataRoot root when root.environment != null: int itemId = int.Parse(Path.GetFileNameWithoutExtension(entry.Name)); yield return (itemId, itemNames.GetValueOrDefault(itemId) ?? string.Empty, root.environment); break; case ItemDataKR rootKR: foreach (var dataRoot in rootKR.items) { if (dataRoot.environment == null) continue; yield return (dataRoot.id, itemNames.GetValueOrDefault(dataRoot.id) ?? string.Empty, dataRoot.environment); } break; + default: + Console.WriteLine($"Unexpected item data type: {typeof(T).Name}"); + break; }Maple2.File.Tests/ItemOptionParserTest.cs (1)
99-106: Consider clarifying or removing commented-out test.The commented-out TestItemOptionPickParserKR method with the note "Moved to constant" could be replaced with a clearer comment explaining where this functionality is now tested.
-// [TestMethod] // Moved to constant -// public void TestItemOptionPickParserKR() { -// var parser = new ItemOptionParser(TestUtilsKR.XmlReader); -// -// foreach (var data in parser.ParsePick()) { -// Assert.IsNotNull(data); -// } -// } +// Note: Option pick parsing for KR is now tested as part of the constant options in TestItemOptionParserKRMaple2.File.Parser/Xml/ItemOptionRandom.cs (1)
1-7: Header comment references the wrong source file
The comment saysitemoptionconstant.xmlbut this model mapsitemoptionrandom.xml.
This will confuse maintainers searching for the mapping.-// ./data/xml/table/itemoptionconstant.xml +// ./data/xml/table/itemoptionrandom.xmlMaple2.File.Parser/MapParser.cs (3)
21-23: Hard‑coded English map‑name file breaks KR localisation
Even in KR mode the parser still loadsen/mapname.xml(line 26).
If a KR‑specific map‑name file exists the names will be wrong or missing.-XmlReader reader = xmlReader.GetXmlReader(xmlReader.GetEntry("en/mapname.xml")); +string nameFile = FeatureLocaleFilter.Locale == "KR" ? "kr/mapname.xml" : "en/mapname.xml"; +XmlReader reader = xmlReader.GetXmlReader(xmlReader.GetEntry(nameFile));Please confirm the correct path and encoding of the KR map‑name file.
32-38: Repeated LINQ allocation can be avoided
xmlReader.Files.Where(entry => entry.Name.StartsWith("map/"));
creates an intermediate collection on every call toParse().
Filesis already enumerable; switching to an iterator (foreach) or embedding the predicate in the loop eliminates the extra allocation.
59-60: UseTryGetValueto avoid double dictionary lookup
mapNames.GetValueOrDefault(mapId) ?? string.Emptyperforms two look‑ups when the key is present.-mapNames.GetValueOrDefault(mapId) ?? string.Empty -mapNames.TryGetValue(mapId, out var name) ? name : string.EmptyMinor, but free performance win when parsing thousands of maps.
Maple2.File.Parser/Xml/Table/ItemMergeOption.cs (1)
89-103: Comprehensive OptionKR class with max value attributes.Instead of arrays, the KR option uses direct max value attributes for each index. This is a good representation of the KR XML structure. Consider adding an indexer similar to the original Option class to make accessing values by index consistent across implementations.
public partial class OptionKR { [XmlAttribute] public string optionName = string.Empty; [XmlAttribute] public int min; [XmlAttribute] public float idx0_max; [XmlAttribute] public float idx1_max; [XmlAttribute] public float idx2_max; [XmlAttribute] public float idx3_max; [XmlAttribute] public float idx4_max; [XmlAttribute] public float idx5_max; [XmlAttribute] public float idx6_max; [XmlAttribute] public float idx7_max; [XmlAttribute] public float idx8_max; [XmlAttribute] public float idx9_max; + + public float GetMaxAt(int i) => i switch { + 0 => idx0_max, + 1 => idx1_max, + 2 => idx2_max, + 3 => idx3_max, + 4 => idx4_max, + 5 => idx5_max, + 6 => idx6_max, + 7 => idx7_max, + 8 => idx8_max, + 9 => idx9_max, + _ => throw new ArgumentOutOfRangeException(nameof(i), i, "Invalid option index."), + }; }Maple2.File.Parser/NpcParser.cs (1)
32-45: Minor: repeatedParseNpcNames()call per parser instance
Parse()andParseKR()each rebuild the same dictionary.
Caching the result in a private, lazily‑initialised field would avoid redundant XML reads when both methods are invoked.Maple2.File.Tests/QuestParserTest.cs (2)
20-28: Hard‑coded expected counts make the test brittleThe assertions
Assert.AreEqual(4903, count)and similar lock the test suite to a specific data‑set version.
A minor data file update will fail the build even if the parser logic is correct.Recommended alternatives:
• Assert thatcountis> 0and maybe within a broad range, or
• Load an expected count from a manifest shipped alongside the XML so tests evolve with the data.
30-48: Duplicate code across NA/KR testsThe four test methods share identical setup & counting logic. Extract the common loop into a private helper and parameterise by locale and expected count to improve maintainability.
Maple2.File.Parser/QuestParser.cs (1)
57-75: Hard‑coded"string/en"path ignores active locale
ParseQuestDescriptions()always reads English description files. For KR builds this yields English names (or none if the files are absent).Consider resolving the folder dynamically:
string langFolder = FeatureLocaleFilter.Locale == "KR" ? "string/kr" : "string/en"; foreach (PackFileEntry entry in xmlReader.Files.Where(e => e.Name.StartsWith($"{langFolder}/questdescription"))) { ... }Maple2.File.Parser/Xml/Table/IndividualItemDrop.cs (1)
85-98: Consider aligning field naming with existing modelExisting (non‑KR) model uses
item,item2,weight, etc.
The KR model switches toitemID,itemID2,uiItemRank, etc. If you intend consumers to handle the two variants generically, you may want to:
- Keep identical property names where semantics match, or
- Introduce an interface / base class so callers don’t need
if (locale…)cascades.Not blocking, but think about long‑term maintainability.
Maple2.File.Parser/Xml/Skill/Kinds.cs (1)
38-68: Reduce duplication betweenKindsandKindsKR.
KindsKRre‑lists ~30 identical fields fromKinds, differing only inimmediateActive’s type (bool→int).
Long‑term this will be a maintenance liability: any future field added toKindsmust be manually copied toKindsKR.Consider extracting the common data into a base record/struct, or re‑using
Kindsvia composition/inheritance and exposing only the truly divergent members:-public partial class KindsKR { - // … 30 duplicated members … - [XmlAttribute] public int immediateActive; // 0,1,2 -} +public partial class KindsKR : Kinds { + // override the single divergent field + [XmlAttribute] public new int immediateActive; // 0,1,2 +}That keeps the serializers intact while eliminating the copy‑paste burden.
Maple2.File.Parser/Xml/Item/Item.cs (1)
55-85: Duplicate property lists hurt maintainability.
ItemDataKRrepeats the full 40‑property list fromItemData. Future edits will require touching both classes.If the schema is truly identical, prefer:
public partial class ItemDataKR : ItemData { /* KR‑only overrides here */ }or share a common interface/record generated from the XSD.
Maple2.File.Parser/Xml/Common/ItemOptionKR.cs (1)
40-75: Large switch blocks → data‑driven table
StatValue(byte i)contains 30caselabels that map indices to backing fields.
The same pattern is repeated for the other three getters, totalling >250caselabels.For readability & JIT friendliness you could replace the switches with static arrays:
-private int StatValue(byte i) => i switch { 0 => str_value_base, … }; +private static readonly Func<ItemOptionKR, int>[] _statValue = +{ + o => o.str_value_base, + o => o.dex_value_base, + // … +}; +public int StatValue(byte i) => + i < _statValue.Length ? _statValue[i](this) + : throw new ArgumentOutOfRangeException(nameof(i), i, null);Benefits:
• far smaller IL,
• easier to audit,
• constant‑time amendments (just reorder the array).Same approach applies to
StatRate,SpecialValue, andSpecialRate.Maple2.File.Parser/TableParser.cs (2)
236-242: Path selection pattern repeats – extract helper.Several methods now follow the “
locale == "kr"? koreanPath : naPath” pattern.
Centralising this into a small helper keeps the parser concise:private string L(string na, string kr) => locale == "kr" ? kr : na; // usage string filename = L("table/na/colorpalette_achieve.xml", "table/colorpalette.xml");This removes ~40
ifblocks across the file.
802-818: Minor: consider eager grouping to avoid repeat enumerations.
ParseIndividualItemDropKR()performs twoGroupBypasses (GroupBythenSelectMany).
MaterialisingdropBox.Groupsinto a dictionary once per drop box can cut allocations:yield return (dropBox.DropBoxID, - dropBox.Groups.GroupBy(g => (byte)g.DropGroupID) - .ToDictionary(g => g.Key, g => g.SelectMany(grp => grp.Items).ToList())); + dropBox.Groups + .GroupBy(g => (byte)g.DropGroupID, g => g.Items) + .ToDictionary(g => g.Key, g => g.SelectMany(i => i).ToList()));Maple2.File.Tests/TableParserKRTest.cs (1)
444-451: Remove redundant API callThe method calls
ParseAdventureLevelAbility()twice, with the first result stored in an unused variableresults. This redundancy is unnecessary and could affect test performance.Apply this diff to remove the redundant call:
[TestMethod] public void TestAdventureLevelAbilityKR() { - var results = _parser.ParseAdventureLevelAbility(); foreach ((_, _) in _parser.ParseAdventureLevelAbility()) { continue; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (54)
Maple2.File.Generator/XmlArrayGenerator.cs(1 hunks)Maple2.File.Parser/Enum/ConditionType.cs(1 hunks)Maple2.File.Parser/Enum/DungeonType.cs(1 hunks)Maple2.File.Parser/Enum/ItemExchangeScrollType.cs(1 hunks)Maple2.File.Parser/Enum/NameTagSymbolConditionType.cs(1 hunks)Maple2.File.Parser/Enum/QuestPropertyType.cs(1 hunks)Maple2.File.Parser/Enum/QuestRewardType.cs(1 hunks)Maple2.File.Parser/ItemOptionParser.cs(5 hunks)Maple2.File.Parser/ItemParser.cs(2 hunks)Maple2.File.Parser/MapParser.cs(3 hunks)Maple2.File.Parser/NpcParser.cs(2 hunks)Maple2.File.Parser/QuestParser.cs(3 hunks)Maple2.File.Parser/RidingParser.cs(2 hunks)Maple2.File.Parser/ScriptParser.cs(4 hunks)Maple2.File.Parser/SkillParser.cs(2 hunks)Maple2.File.Parser/TableParser.cs(34 hunks)Maple2.File.Parser/Tools/Sanitizer.cs(1 hunks)Maple2.File.Parser/Xml/Common/ItemOptionKR.cs(1 hunks)Maple2.File.Parser/Xml/Item/Item.cs(1 hunks)Maple2.File.Parser/Xml/Item/Property.cs(1 hunks)Maple2.File.Parser/Xml/ItemOptionConstant.cs(1 hunks)Maple2.File.Parser/Xml/ItemOptionRandom.cs(1 hunks)Maple2.File.Parser/Xml/Map/Map.cs(1 hunks)Maple2.File.Parser/Xml/Npc/Npc.cs(1 hunks)Maple2.File.Parser/Xml/Quest/Quest.cs(1 hunks)Maple2.File.Parser/Xml/Riding/Riding.cs(1 hunks)Maple2.File.Parser/Xml/Script/NpcScript.cs(1 hunks)Maple2.File.Parser/Xml/Skill/Basic.cs(1 hunks)Maple2.File.Parser/Xml/Skill/Kinds.cs(1 hunks)Maple2.File.Parser/Xml/Skill/Skill.cs(2 hunks)Maple2.File.Parser/Xml/Table/DungeonRoom.cs(1 hunks)Maple2.File.Parser/Xml/Table/IndividualItemDrop.cs(1 hunks)Maple2.File.Parser/Xml/Table/ItemMergeOption.cs(1 hunks)Maple2.File.Parser/Xml/Table/JobTable.cs(1 hunks)Maple2.File.Parser/Xml/Table/SetItemOption.cs(1 hunks)Maple2.File.Tests/AdditionalEffectParserTest.cs(3 hunks)Maple2.File.Tests/AiParserTest.cs(1 hunks)Maple2.File.Tests/FunctionCubeParserTest.cs(2 hunks)Maple2.File.Tests/ItemOptionParserTest.cs(6 hunks)Maple2.File.Tests/ItemParserTest.cs(2 hunks)Maple2.File.Tests/MapParserTest.cs(2 hunks)Maple2.File.Tests/NifParserTest.cs(2 hunks)Maple2.File.Tests/NpcParserTest.cs(2 hunks)Maple2.File.Tests/QuestParserTest.cs(2 hunks)Maple2.File.Tests/RidingParserTest.cs(4 hunks)Maple2.File.Tests/ScriptParserTest.cs(5 hunks)Maple2.File.Tests/ServerTableParserTest.cs(1 hunks)Maple2.File.Tests/SkillParserTest.cs(3 hunks)Maple2.File.Tests/TableParserKRTest.cs(1 hunks)Maple2.File.Tests/TableParserTest.cs(2 hunks)Maple2.File.Tests/TestUtils.cs(1 hunks)Maple2.File.Tests/XBlockParserTest.cs(3 hunks)Maple2.File.sln(1 hunks)global.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
Maple2.File.Tests/ItemOptionParserTest.cs (1)
Learnt from: AngeloTadeucci
PR: AngeloTadeucci/Maple2.File#36
File: Maple2.File.Tests/TableParserTest.cs:777-783
Timestamp: 2024-10-09T04:13:28.776Z
Learning: In the 'Maple2.File' project, tests are designed to ensure methods can be called without errors; do not suggest adding assertions or validations in test methods.
🧬 Code Graph Analysis (17)
Maple2.File.Parser/Xml/Script/NpcScript.cs (2)
Maple2.File.Parser/Xml/Npc/Npc.cs (2)
XmlRoot(7-11)XmlRoot(42-45)Maple2.File.Parser/Xml/Script/QuestScript.cs (1)
XmlRoot(7-10)
Maple2.File.Tests/SkillParserTest.cs (4)
Maple2.File.Parser/Tools/Filter.cs (1)
Filter(9-47)Maple2.File.Tests/TestUtils.cs (4)
TestUtils(11-52)TestUtils(19-43)TestUtilsKR(55-96)TestUtilsKR(63-87)Maple2.File.Parser/SkillParser.cs (3)
SkillParser(12-67)SkillParser(17-22)Dictionary(53-66)Maple2.File.Parser/Xml/Skill/Skill.cs (1)
SkillKR(148-163)
Maple2.File.Parser/RidingParser.cs (3)
Maple2.File.Parser/Xml/Riding/Riding.cs (1)
RidingKR(28-30)Maple2.File.IO/M2dReader.cs (1)
GetString(64-67)Maple2.File.Parser/Tools/Sanitizer.cs (2)
Sanitizer(5-105)RemoveEmpty(85-87)
Maple2.File.Parser/ItemOptionParser.cs (5)
Maple2.File.Parser/Xml/ItemOptionConstant.cs (1)
ItemOptionConstant(35-39)Maple2.File.Parser/Tools/Sanitizer.cs (3)
Sanitizer(5-105)RemoveEmpty(85-87)RemoveUtf8Bom(99-104)Maple2.File.IO/M2dReader.cs (2)
GetString(64-67)XmlReader(43-45)Maple2.File.Parser/Xml/ItemOptionRandom.cs (1)
ItemOptionRandomKR(9-18)Maple2.File.Parser/Xml/Table/ItemMergeOption.cs (1)
MergeOptionKR(76-80)
Maple2.File.Parser/Xml/Quest/Quest.cs (4)
Maple2.File.Parser/Xml/Item/Item.cs (2)
XmlRoot(7-10)XmlRoot(45-48)Maple2.File.Parser/Xml/Npc/Npc.cs (2)
XmlRoot(7-11)XmlRoot(42-45)Maple2.File.Parser/Xml/Skill/Skill.cs (2)
XmlRoot(10-14)XmlRoot(142-146)Maple2.File.Parser/Xml/Script/QuestScript.cs (1)
XmlRoot(7-10)
Maple2.File.Parser/Xml/Table/ItemMergeOption.cs (1)
Maple2.File.Parser/Xml/Table/ItemOptionVariation.cs (2)
XmlRoot(7-20)XmlRoot(23-70)
Maple2.File.Tests/XBlockParserTest.cs (2)
Maple2.File.Tests/TestUtils.cs (2)
TestUtilsKR(55-96)TestUtilsKR(63-87)Maple2.File.Parser/MapXBlock/XBlockParser.cs (4)
XBlockParser(10-174)XBlockParser(20-27)ParseMap(59-67)IMapEntity(120-163)
Maple2.File.Tests/FunctionCubeParserTest.cs (3)
Maple2.File.Parser/Tools/Filter.cs (1)
Filter(9-47)Maple2.File.Tests/TestUtils.cs (4)
TestUtils(11-52)TestUtils(19-43)TestUtilsKR(55-96)TestUtilsKR(63-87)Maple2.File.Parser/FunctionCubeParser.cs (2)
FunctionCubeParser(11-33)FunctionCubeParser(15-18)
Maple2.File.Parser/QuestParser.cs (8)
Maple2.File.Generator/Resource/FeatureLocaleFilter.cs (1)
FeatureLocaleFilter(7-76)Maple2.File.Parser/ItemParser.cs (1)
IEnumerable(23-48)Maple2.File.Parser/MapParser.cs (1)
IEnumerable(25-61)Maple2.File.Parser/NpcParser.cs (3)
IEnumerable(32-45)IEnumerable(47-61)Dictionary(25-30)Maple2.File.Parser/AchieveParser.cs (1)
IEnumerable(23-41)Maple2.File.Parser/Xml/Quest/Quest.cs (2)
QuestData(17-54)QuestDataKR(62-64)Maple2.File.IO/M2dReader.cs (3)
PackFileEntry(39-41)XmlReader(43-45)GetString(64-67)Maple2.File.Parser/Tools/Sanitizer.cs (2)
Sanitizer(5-105)SanitizeQuest(61-65)
Maple2.File.Parser/Xml/Npc/Npc.cs (3)
Maple2.File.Parser/Xml/Item/Item.cs (2)
XmlRoot(7-10)XmlRoot(45-48)Maple2.File.Parser/Xml/Script/NpcScript.cs (2)
XmlRoot(7-13)XmlRoot(15-18)Maple2.File.Parser/Xml/Npc/Dummy.cs (1)
EffectDummy(7-9)
Maple2.File.Parser/Xml/ItemOptionRandom.cs (8)
Maple2.File.Parser/Xml/Item/Item.cs (2)
XmlRoot(7-10)XmlRoot(45-48)Maple2.File.Parser/Xml/Map/Map.cs (2)
XmlRoot(7-10)XmlRoot(31-35)Maple2.File.Parser/Xml/ItemOptionConstant.cs (2)
XmlRoot(8-11)XmlRoot(30-33)Maple2.File.Parser/Xml/Riding/Riding.cs (2)
XmlRoot(8-11)XmlRoot(23-26)Maple2.File.Parser/Xml/Quest/Quest.cs (2)
XmlRoot(8-11)XmlRoot(57-60)Maple2.File.Parser/Xml/Script/NpcScript.cs (2)
XmlRoot(7-13)XmlRoot(15-18)Maple2.File.Parser/Xml/Npc/Npc.cs (2)
XmlRoot(7-11)XmlRoot(42-45)Maple2.File.Parser/Xml/Table/ItemMergeOption.cs (1)
XmlRoot(8-11)
Maple2.File.Parser/Xml/Table/SetItemOption.cs (2)
Maple2.File.Parser/Xml/ItemOptionRandom.cs (1)
XmlRoot(4-7)Maple2.File.Parser/Xml/Table/SetItemInfo.cs (1)
XmlRoot(7-10)
Maple2.File.Parser/Xml/Table/IndividualItemDrop.cs (6)
Maple2.File.Parser/Xml/ItemOptionRandom.cs (1)
XmlRoot(4-7)Maple2.File.Parser/Xml/Map/Map.cs (2)
XmlRoot(7-10)XmlRoot(31-35)Maple2.File.Parser/Xml/ItemOptionConstant.cs (2)
XmlRoot(8-11)XmlRoot(30-33)Maple2.File.Parser/Xml/Quest/Quest.cs (2)
XmlRoot(8-11)XmlRoot(57-60)Maple2.File.Parser/Xml/Script/NpcScript.cs (2)
XmlRoot(7-13)XmlRoot(15-18)Maple2.File.Parser/Xml/Npc/Npc.cs (2)
XmlRoot(7-11)XmlRoot(42-45)
Maple2.File.Tests/NpcParserTest.cs (4)
Maple2.File.Parser/Tools/Filter.cs (2)
Filter(9-47)Load(12-46)Maple2.File.Tests/TestUtils.cs (4)
TestUtils(11-52)TestUtils(19-43)TestUtilsKR(55-96)TestUtilsKR(63-87)Maple2.File.Parser/NpcParser.cs (2)
NpcParser(13-62)NpcParser(18-23)Maple2.File.Parser/Xml/Npc/Dummy.cs (1)
EffectDummy(7-9)
Maple2.File.Parser/Xml/Riding/Riding.cs (10)
Maple2.File.Parser/Xml/Item/Item.cs (2)
XmlRoot(7-10)XmlRoot(45-48)Maple2.File.Parser/Xml/ItemOptionRandom.cs (1)
XmlRoot(4-7)Maple2.File.Parser/Xml/Map/Map.cs (2)
XmlRoot(7-10)XmlRoot(31-35)Maple2.File.Parser/Xml/ItemOptionConstant.cs (2)
XmlRoot(8-11)XmlRoot(30-33)Maple2.File.Parser/Xml/Quest/Quest.cs (2)
XmlRoot(8-11)XmlRoot(57-60)Maple2.File.Parser/Xml/Script/NpcScript.cs (2)
XmlRoot(7-13)XmlRoot(15-18)Maple2.File.Parser/Xml/Npc/Npc.cs (1)
XmlRoot(7-11)Maple2.File.Parser/Xml/Riding/PassengerRiding.cs (2)
XmlRoot(8-11)PassengerRiding(13-30)Maple2.File.Parser/Xml/Table/MapSpawnTag.cs (1)
XmlRoot(7-22)Maple2.File.Parser/Xml/Table/PetSpawnInfo.cs (1)
XmlRoot(6-9)
Maple2.File.Parser/SkillParser.cs (7)
Maple2.File.Generator/Resource/FeatureLocaleFilter.cs (1)
FeatureLocaleFilter(7-76)Maple2.File.Parser/ItemParser.cs (1)
IEnumerable(23-48)Maple2.File.Parser/MapParser.cs (1)
IEnumerable(25-61)Maple2.File.Parser/NpcParser.cs (3)
IEnumerable(32-45)IEnumerable(47-61)Dictionary(25-30)Maple2.File.Parser/AdditionalEffectParser.cs (1)
IEnumerable(18-29)Maple2.File.IO/M2dReader.cs (1)
PackFileEntry(39-41)Maple2.File.Parser/Xml/Skill/Skill.cs (1)
SkillKR(148-163)
Maple2.File.Parser/Xml/Table/JobTable.cs (13)
Maple2.File.Parser/Xml/Script/NpcScript.cs (2)
XmlRoot(7-13)XmlRoot(15-18)Maple2.File.Parser/Xml/Skill/Skill.cs (2)
XmlRoot(10-14)XmlRoot(142-146)Maple2.File.Parser/Xml/Table/DefaultItems.cs (1)
XmlRoot(7-30)Maple2.File.Parser/Xml/Script/QuestScript.cs (1)
XmlRoot(7-10)Maple2.File.Parser/Xml/Table/EnchantScroll.cs (1)
XmlRoot(7-10)Maple2.File.Parser/Xml/Table/Fish.cs (1)
XmlRoot(7-10)Maple2.File.Parser/Xml/Table/FishingSpot.cs (1)
XmlRoot(7-10)Maple2.File.Parser/Xml/Table/GuildNpc.cs (1)
XmlRoot(8-11)Maple2.File.Parser/Xml/Table/GuildProperty.cs (1)
XmlRoot(7-10)Maple2.File.Parser/Xml/Table/ItemSocket.cs (1)
XmlRoot(7-10)Maple2.File.Parser/Xml/Table/ItemSocketScroll.cs (1)
XmlRoot(7-10)Maple2.File.Parser/Xml/Table/Feature.cs (1)
XmlRoot(6-9)Maple2.File.Parser/Xml/Table/FeatureSetting.cs (1)
XmlRoot(6-9)
🔇 Additional comments (76)
Maple2.File.Tests/AiParserTest.cs (1)
124-124: Approve formatting adjustment
The addition of a trailing newline at the end of the file is a harmless formatting improvement that aligns with POSIX text file conventions.global.json (1)
1-6: LGTM: SDK configuration ensures development environment consistencyThis global.json configuration ensures all developers use consistent .NET SDK versions by rolling forward to the latest major version and allowing prereleases when needed.
Maple2.File.Parser/Enum/DungeonType.cs (1)
42-42: LGTM: Appropriate alias for locale supportThe
killalias for theKillenum value provides case-insensitive compatibility for XML deserialization, which is useful for supporting different locale formats.Maple2.File.Parser/Xml/Skill/Basic.cs (1)
12-16: LGTM: Well-structured Korean locale support classThe
BasicKRclass correctly implements theIFeatureLocaleinterface and follows the same structure as the originalBasicclass while using the appropriateKindsKRtype for Korean locale support.Maple2.File.Parser/Tools/Sanitizer.cs (1)
99-104: LGTM: Useful UTF-8 BOM removal utilityThis method effectively removes the UTF-8 BOM character from the beginning of XML strings, which is important for proper parsing of files from different sources, especially Korean locale files that may include this marker.
Maple2.File.sln (1)
16-21: Solution structure update looks good.Added a "Solution Items" folder with environment files that will help manage locale-specific configurations. This addition aligns well with the KR XML parsing feature being implemented.
Maple2.File.Parser/Xml/Map/Map.cs (2)
30-35: Clear documentation of XML structure.The comment clearly identifies the XML file path for KR data format. The class structure appropriately reflects the different XML organization compared to the non-KR format.
37-40: Well-structured KR-specific mapping.The partial class implementation with ID attribute and feature locale annotation follows the same pattern used in the non-KR version, which provides consistency in the codebase.
Maple2.File.Tests/TestUtils.cs (1)
69-87: Identical M2dReader initialization pattern.The file paths and reader initialization for KR data follows the same pattern as the non-KR version, maintaining consistency.
Maple2.File.Parser/Xml/Table/SetItemOption.cs (3)
31-35: Clear XML path documentation and appropriate root class.The comment specifies the XML file path, and the root class structure with
[XmlRoot("ms2")]follows the same pattern as other XML serialization classes. Theoptionlist using the "set" XML element correctly models the KR-specific format.
37-42: Well-defined KR item option structure.The
SetItemOptionKRclass properly implementsIFeatureLocaleand includes the necessary attributes for the Korean format. TheitemIDsarray initialization with= []ensures it's never null.
44-54: Consistent implementation of nested Part class.The nested
Partclass appropriately inherits fromItemOptionKR(instead ofItemOption) to match the Korean data format. All attributes mirror the non-KR version, maintaining consistency between the implementations.Maple2.File.Tests/ServerTableParserTest.cs (1)
439-439: Skip: Trailing newline addition
No functional changes; only a trailing newline was added.Maple2.File.Parser/Enum/QuestPropertyType.cs (1)
7-7: New enum member added:hideworldmap
The addition ofhideworldmap = 3follows the existing value sequence and naming style. Please verify that the quest parser and its tests have been updated to handle this new property.Maple2.File.Parser/Enum/ItemExchangeScrollType.cs (1)
8-8: New enum member added:upgrade
Addingupgrade = 4is consistent with other scroll types. Ensure that any parsing logic or switch statements are updated accordingly and covered by tests.Maple2.File.Parser/Enum/NameTagSymbolConditionType.cs (1)
16-16: New enum member added:return_user
The newreturn_userentry aligns with the overall enum updates. Confirm that any consumers ofNameTagSymbolConditionType(parsers, serializers, UI mappings) have been extended and tested.Maple2.File.Parser/Enum/QuestRewardType.cs (1)
7-7: LGTM - Added capitalized alias for consistent enum handlingThe addition of
SkillPointas an alias forskillPoint(sharing the same value2) seems to be part of supporting Korean locale data parsing where the capitalization might differ in the XML.Maple2.File.Parser/Xml/Table/DungeonRoom.cs (1)
18-18: Attribute changed from [XmlAttribute] to [M2dEnum] for improved enum handlingThe change from
[XmlAttribute]to[M2dEnum]for thegroupTypeproperty allows for more specialized handling of enum serialization/deserialization, which is consistent with the broader KR XML parsing enhancements in this PR.Maple2.File.Generator/XmlArrayGenerator.cs (1)
69-69: Improved handling of null arrays in XML serializationChanged from returning
nullto returningstring.Emptywhen serializing null array fields. This prevents potential XML serialization issues and ensures more consistent behavior across different locales.Maple2.File.Tests/XBlockParserTest.cs (1)
38-53: LGTM - Added KR version of XBlock parser testThe new test method
TestXBlockParserKRcorrectly uses the Korean data sources throughTestUtilsKR.ExportedReader, maintaining parity with the existingTestXBlockParsermethod. This addition properly validates that XBlock parsing works for Korean map data.Maple2.File.Parser/Xml/Script/NpcScript.cs (1)
15-22: Good implementation of KR-specific classes for NPC scriptsThe implementation correctly follows the pattern established across the codebase for Korean locale support. The
NpcScriptListKRserves as the root container for multiple NPC scripts in the consolidated KR format, whileNpcScriptKRextends the baseNpcScriptclass to add the necessaryidattribute for identification within the consolidated file.Maple2.File.Parser/Xml/Riding/Riding.cs (1)
22-30: Well-structured implementation for Korean riding dataThe implementation correctly extends the riding model to support the Korean format which includes passenger information. The inheritance hierarchy is appropriate -
RidingKRextendsRidingto inherit all base properties while adding the KR-specificpassengerslist. The XML structure is properly defined with appropriate root and element annotations.Maple2.File.Parser/Xml/Quest/Quest.cs (1)
57-64: Good implementation of KR-specific quest data classesThe implementation correctly follows the pattern established across the codebase for Korean locale support. The
QuestDataRootKRserves as the root container for multiple quests in the consolidated KR format, whileQuestDataKRextends the baseQuestDataclass to add the necessaryidattribute for identification. The implementation ofIFeatureLocaleinterface inQuestDataKRis appropriate for locale-specific filtering.Maple2.File.Tests/FunctionCubeParserTest.cs (1)
12-25: Good addition of locale filter and validationThe changes to the existing test method correctly add locale filtering initialization and validate the expected count of function cube entries, which improves the test's thoroughness and consistency with other test methods in the codebase.
Maple2.File.Tests/AdditionalEffectParserTest.cs (2)
12-12: LGTM! Proper filter loading added before parser creation.Great addition of the filter loading for North American region, ensuring the parser operates with the correct locale context.
24-36: Well-structured test for KR locale data parsing.This test method follows the appropriate pattern for testing locale-specific functionality. It correctly:
- Loads the KR filter
- Uses the Korean XML reader
- Applies the same validation as the NA version
- Verifies the expected count (6070) of parsed items
The test ensures the AdditionalEffectParser correctly handles Korean locale data.
Maple2.File.Tests/TableParserTest.cs (1)
10-16: Good refactoring to static parser instance.Excellent change to use a single static TableParser instance initialized once per test class. This optimization:
- Reduces redundancy by avoiding parser recreation for each test method
- Improves test efficiency
- Ensures consistent filter configuration across all tests
Maple2.File.Tests/RidingParserTest.cs (2)
12-12: LGTM! Filter loading properly added.Good addition of Filter.Load calls before parser creation, ensuring correct locale context for both test methods.
Also applies to: 27-28
40-58: Comprehensive KR riding test with proper passenger validation.Well-structured test that:
- Correctly loads the KR filter and uses the Korean XML reader
- Tests the new ParseKR() method
- Includes thorough validation for passenger objects
- Verifies the expected count of 615 riding entries
The additional validation for passengers is particularly good - it ensures that complex nested data structures are properly parsed.
Maple2.File.Tests/MapParserTest.cs (2)
12-12: LGTM! Added filter loading and count verification.Good additions including:
- Filter loading with proper region parameters
- Counting parsed maps
- Asserting the exact expected count (1200)
These changes ensure the parser operates with the correct locale context and validates the expected dataset size.
Also applies to: 20-27
30-47: Well-structured test for KR map data.This test method follows the appropriate pattern for testing locale-specific functionality:
- Loads the KR filter
- Uses the Korean XML reader
- Applies thorough validation including ID and nullability checks
- Verifies that exactly 1299 maps are parsed from the Korean dataset
The commented-out event handlers maintain consistency with the NA test method.
Maple2.File.Parser/RidingParser.cs (2)
15-15: Field added to support Korean locale parsing.The
ridingKRSerializerfield is added to deserialize the Korean-specific riding data structure.
21-21: Constructor initialized with Korean-specific serializer.The serializer is correctly initialized with the
RidingKRRoottype, consistent with the pattern used for other serializers.Maple2.File.Parser/Xml/Npc/Npc.cs (3)
42-45: Added Korean-specific NPC data list class.The
NpcDataListKRclass is correctly defined with the "ms2" XML root and contains a list ofNpcDataRootKRobjects. This follows the same pattern used for other Korean-specific data structures.
47-50: Added Korean-specific NPC data root class.The
NpcDataRootKRclass properly defines the structure for individual NPC entries with an ID attribute and environment element. This matches the expected XML structure for Korean NPC data.
52-54:Details
❌ Incorrect review comment
Potential duplication of
effectdummyproperty.The
NpcDataKRclass adds aneffectdummyproperty, but the regularNpcDataRootclass already has this property (line 10). This suggests a potential structural inconsistency between the regular and Korean XML formats.
🏁 Script executed:
#!/bin/bash # Check both data structures to understand the difference in effectdummy placement echo "Checking NpcDataRoot effectdummy usage:" rg -A 3 -B 3 "effectdummy" --glob "*.cs"Length of output: 2421
Ignore the reported duplication of
effectdummy.The two
effectdummyproperties live in separate XML contexts by design:
NpcDataRoot.effectdummymaps the<effectdummy>element at the root level for the default format.NpcDataKR.effectdummymaps the<effectdummy>element under the<environment>node for the Korean‐locale format.The parser correctly dispatches to
root.effectdummyfor default data and todataKr.effectdummyfor KR data, so there is no structural inconsistency.Likely an incorrect or invalid review comment.
Maple2.File.Tests/ScriptParserTest.cs (3)
12-12: Explicit filter loading for NA locale.The addition of explicit filter loading with "NA" and "Live" parameters ensures consistent test behavior across different environments and locales.
Also applies to: 31-31, 50-50
68-80: Test added for Korean NPC script parsing.The test properly validates the Korean NPC script parsing functionality by:
- Loading the correct filter for the KR locale
- Using the appropriate XML reader
- Verifying valid IDs and non-null scripts
- Confirming the expected count of 3271 scripts
This test ensures the new Korean-specific parsing functionality works correctly.
82-94: Test added for Korean Quest script parsing.The test properly validates the Korean Quest script parsing functionality by:
- Loading the correct filter for the KR locale
- Using the appropriate XML reader
- Verifying valid IDs and non-null scripts
- Confirming the expected count of 4291 scripts
This test ensures the new Korean-specific parsing functionality works correctly.
Maple2.File.Parser/ItemParser.cs (1)
19-20: Conditional serializer initialization based on locale.The
ItemSerializeris now correctly initialized based on the current locale, usingItemDataKRfor the Korean locale andItemDataRoototherwise.Maple2.File.Tests/SkillParserTest.cs (5)
2-2: Good addition for filter support.Adding the Tools namespace import is necessary for the Filter.Load calls added throughout the file.
12-12: Nice initialization of locale and environment.Setting up the Filter with appropriate "NA" and "Live" parameters before parsing ensures consistent test execution.
24-36: Well-structured KR skill parser test.This test follows the same pattern as the NA test but correctly uses TestUtilsKR.XmlReader with "KR" locale, and calls the specialized ParseKR() method. The assertions verify positive IDs, non-null data, and the expected count of skills.
39-46: Good test for NA skill name loading.This test isolates and verifies the LoadSkillNames functionality for the NA locale with the appropriate expected count.
48-55: Good test for KR skill name loading.This test follows the same pattern as the NA name loading test but correctly uses TestUtilsKR.XmlReader with "KR" locale and verifies the expected count of Korean skill names.
Maple2.File.Parser/ScriptParser.cs (4)
3-3: New import for XML generation.Adding the M2dXmlGenerator namespace import is necessary for XML serialization functionality.
19-19: Nice conditional XML serializer initialization.This change enables the ScriptParser to deserialize either NpcScriptListKR for Korean locale or NpcScript for other locales based on the FeatureLocaleFilter.Locale value.
34-43: Well-implemented Korean NPC script parsing.This method correctly handles the different structure of Korean NPC script data by:
- Looking for a single consolidated XML file ("npcscript_final.xml") instead of multiple files
- Deserializing into the appropriate NpcScriptListKR type
- Yielding individual NPC scripts with their IDs
The use of Debug.Assert ensures proper error detection during parsing.
56-65: Well-implemented Korean quest script parsing.This method follows the same pattern as ParseNpcKR by targeting a single consolidated XML file ("questscript_final.xml") for Korean quest scripts, deserializing it, and yielding individual quest scripts.
Maple2.File.Tests/ItemParserTest.cs (4)
2-2: Good addition for filter support.Adding the Tools namespace import is necessary for the Filter.Load calls added throughout the file.
12-12: Nice initialization of locale and environment.Setting up the Filter with appropriate "NA" and "Live" parameters before parsing ensures consistent test execution.
20-28: Good implementation of item count tracking and verification.Adding a counter to track the total number of parsed items and asserting the expected count provides stronger validation of the parsing process.
31-49: Well-structured KR item parser test.This test follows the same pattern as the NA test but correctly uses TestUtilsKR.XmlReader with "KR" locale and parses with ItemDataKR. The assertions verify positive IDs, non-null data, and the expected count of items.
Maple2.File.Tests/ItemOptionParserTest.cs (5)
2-2: Good addition for filter support.Adding the Tools namespace import is necessary for the Filter.Load calls added throughout the file.
11-11: Nice initialization of locale and environment in all test methods.Setting up the Filter with appropriate "NA" and "Live" parameters before parsing in each test method ensures consistent test execution.
Also applies to: 27-27, 40-40, 50-50
61-84: Comprehensive test for Korean item options parsing.This test properly validates the KR-specific parsing methods for constant and random options, including count verification. The commented section about static options provides important context that this option type may not exist in the Korean version.
86-97: Good test for Korean merge option parsing.This test correctly validates the ParseMergeOptionBaseKR method with appropriate assertions on data and count.
108-119: Good test for Korean option variation parsing.This test correctly validates the variation and variation equipment parsing for Korean data.
Maple2.File.Tests/NifParserTest.cs (1)
24-25: Good move to remove duplication
Delegating the heavy‑weight validation to a single helper makes the individual tests concise and easier to extend.Maple2.File.Tests/NpcParserTest.cs (5)
12-12: Good use of the Filter class to ensure correct locale.Adding the explicit locale loading ensures the proper context is set before parsing, which is especially important when testing both NA and KR data.
20-27: Improved test structure with proper count validation.The updated test now has a proper count assertion to verify the exact number of NPCs parsed (7535), which is a good enhancement to the test coverage.
30-48: Well-structured KR parser test.The Korean NPC parser test follows the same pattern as the NA test, with appropriate use of KR-specific utilities and assertions. The count verification (10795) ensures the correct number of entries are parsed.
50-63: Good addition of NPC name parsing test.This test focuses specifically on parsing NPC names from the NA dataset, providing better test coverage for that specific functionality.
65-78: Comprehensive KR locale name parser test.This test completes the test matrix by covering KR locale name parsing, maintaining consistency with the other test methods.
Maple2.File.Parser/ItemOptionParser.cs (3)
53-56: Good addition of KR-specific serializers.The new serializer fields follow the naming convention of the existing ones and prepare for KR-specific XML deserialization.
65-68: Proper initialization of new serializers.The initialization of KR-specific serializers with their corresponding types looks correct.
157-167: Good implementation of ParseMergeOptionBaseKR method.This method correctly handles a single file without unnecessary looping.
Maple2.File.Parser/Xml/Table/ItemMergeOption.cs (3)
72-74: Well-structured root class for KR item merge options.The ItemMergeOptionRootKR class follows the same XML root structure as the original class, correctly mapping to the KR XML format.
76-80: KR merge option class with simpler structure.The MergeOptionKR class has a simpler structure than the original, directly containing Slot elements without the intermediate MergeStep class. This matches the KR XML structure.
82-87: Well-defined Slot class with option elements.The Slot class is properly extended to include KR-specific option elements.
Maple2.File.Parser/Xml/ItemOptionConstant.cs (4)
30-33: Well-defined root class for KR item option constants.The new ItemOptionConstantRootKR class correctly maps to the KR XML structure with an XML element list of options.
35-39: Clear item option constant class for KR format.The ItemOptionConstant class for KR data includes the code attribute and a list of ranks, properly reflecting the hierarchical structure in the KR XML.
41-46: Well-structured rank class with appropriate attributes.The ItemOptionConstantRank class includes grade and createType attributes and contains a list of option values, correctly mapping to the KR XML structure.
48-52: Comprehensive data class with detailed documentation.The ItemOptionConstantDataKR class includes all necessary attributes and a helpful comment documenting all possible name values. This is excellent for future maintenance.
Maple2.File.Parser/QuestParser.cs (1)
21-23:Details
❓ Verification inconclusive
questSerializertype mismatch whenLocale == "KR"When the locale is KR you instantiate the serializer with
QuestDataRootKR, yet theParse()method below still down‑casts the result toQuestDataRootRoot.
CallingParse()whileLocale == "KR"will therefore returnnulland trigger theDebug.Assert.Two options:
- Guard the method:
if (FeatureLocaleFilter.Locale == "KR") throw new InvalidOperationException("Use ParseKR() for the KR locale");
- Or create two dedicated serializers instead of one polymorphic field.
Pick whichever aligns better with the public surface you want to expose.
#!/bin/bash set -e # Locate QuestParser.cs file=$(fd --absolute-path QuestParser.cs) echo "Inspecting: $file" # Show serializer instantiation around constructor rg -n "XmlSerializer" -A3 "$file" # Show the Parse method and its casting logic rg -n "Parse(" -A10 "$file"
Address serializer mismatch for KR locale in QuestParser.cs
The constructor at Maple2.File.Parser/QuestParser.cs (around lines 21–23)
Type type = FeatureLocaleFilter.Locale == "KR" ? typeof(QuestDataRootKR) : typeof(QuestDataRootRoot); questSerializer = new XmlSerializer(type);always assigns a serializer for
QuestDataRootKRwhenLocale == "KR", but theParse()method below still casts the result toQuestDataRootRoot, leading to anulland a triggeredDebug.Assert.You’ll need to choose one of the following approaches to keep your public API consistent:
- Guard the default
Parse()method
Immediately reject KR locale inParse()so callers use a dedicated KR parser:// Inside Parse() + if (FeatureLocaleFilter.Locale == "KR") + throw new InvalidOperationException("Use ParseKR() for the KR locale");- Maintain two serializers and parsing methods
Introduce separate fields (e.g.questSerializerDefaultandquestSerializerKR) and correspondingParse()/ParseKR()methods that return the correct root type without down‑casting.Maple2.File.Parser/QuestParser.cs:
- Constructor: serializer instantiation (lines 21–23)
Parse()method: casting logic needs updatingPick the strategy that best aligns with your intended public surface.
Maple2.File.Parser/Xml/Table/JobTable.cs (1)
70-88: InconsistentdefaultWeaponSlottype between NA and KR models
JobTable.defaultWeaponSlotisstring[]while the KR counterpart is a singleint.
If this mirrors the real XML schema that’s fine, but downstream code that expects an array will break when fed a KR object.Ensure all parsing/consuming code branches on locale before dereferencing this field, or standardise the property to one unified shape (e.g., always expose an
int[]and map appropriately during deserialization).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Maple2.File.Tests/TableParserKRTest.cs (2)
20-656: 🛠️ Refactor suggestionEnhance test assertions for most test methods
Most test methods only iterate over parsing results without making assertions about the expected data. While this verifies no exceptions are thrown, it doesn't validate that the data is correct or complete.
Consider enhancing at least some of the critical test methods with assertions that verify:
- The expected number of items is parsed
- Key objects contain expected values
- Required properties are not null or empty
For example, similar to how
TestParseSetItemOptionKr(lines 362-377) andTestIndividualItemDropKr(lines 386-393) include count assertions.Here's a template for implementing improved assertions:
[TestMethod] public void TestExampleImprovedMethod() { int count = 0; foreach ((int id, var item) in _parser.ParseExample()) { count++; // Add assertions for important items if (id == knownItemId) { Assert.IsNotNull(item.ImportantProperty); Assert.AreEqual(expectedValue, item.SomeProperty); } } // Verify total count is as expected Assert.IsTrue(count > 0, "No items were parsed"); // If the exact count is stable, assert it // Assert.AreEqual(expectedCount, count); }
165-169:⚠️ Potential issueFix method call in TestGuildExpKr
The test method
TestGuildExpKrincorrectly callsParseGuildEvent()instead of what appears to be the intendedParseGuildExp(). This is likely a copy-paste error from the previous test method.Apply this diff to fix the method call:
[TestMethod] public void TestGuildExpKr() { - foreach ((_, _) in _parser.ParseGuildEvent()) { + foreach ((_, _) in _parser.ParseGuildExp()) { continue; } }
🧹 Nitpick comments (1)
Maple2.File.Tests/TableParserKRTest.cs (1)
286-290: Remove obsolete assertions in TestParseJobTableKrThese commented-out assertions appear to reference properties (
learn,startInvenItem,reward) that don't exist on theJobTableKRclass or have a different structure in the Korean implementation.Remove these obsolete commented-out assertions to keep the code clean and prevent confusion:
Assert.IsTrue(job[0].skills.skills.Count > 0); - // Assert.IsTrue(job[0].learn.Count > 0); - // // Ensure the right amount of items are parsed - // Assert.AreEqual(job[0].startInvenItem.item.Count, itemCount); - // Assert.AreEqual(job[0].reward.item.Count, itemCount);If you need to verify specific properties of the Korean job table, add new assertions for the actually available properties (e.g.,
job[0].startItems.Length).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
Maple2.File.Parser/ItemOptionParser.cs(6 hunks)Maple2.File.Parser/ItemParser.cs(2 hunks)Maple2.File.Parser/MapParser.cs(2 hunks)Maple2.File.Parser/NpcParser.cs(2 hunks)Maple2.File.Parser/PetParser.cs(1 hunks)Maple2.File.Parser/QuestParser.cs(3 hunks)Maple2.File.Parser/RidingParser.cs(2 hunks)Maple2.File.Parser/ScriptParser.cs(4 hunks)Maple2.File.Parser/SkillParser.cs(2 hunks)Maple2.File.Parser/Xml/Map/Map.cs(1 hunks)Maple2.File.Parser/Xml/Script/NpcScript.cs(1 hunks)Maple2.File.Tests/AdditionalEffectParserTest.cs(3 hunks)Maple2.File.Tests/FunctionCubeParserTest.cs(2 hunks)Maple2.File.Tests/ItemOptionParserTest.cs(6 hunks)Maple2.File.Tests/ItemParserTest.cs(2 hunks)Maple2.File.Tests/MapParserTest.cs(2 hunks)Maple2.File.Tests/NifParserTest.cs(2 hunks)Maple2.File.Tests/NpcParserTest.cs(2 hunks)Maple2.File.Tests/QuestParserTest.cs(2 hunks)Maple2.File.Tests/RidingParserTest.cs(4 hunks)Maple2.File.Tests/ScriptParserTest.cs(5 hunks)Maple2.File.Tests/SkillParserTest.cs(3 hunks)Maple2.File.Tests/TableParserKRTest.cs(1 hunks)Maple2.File.Tests/TestUtils.cs(1 hunks)Maple2.File.Tests/XBlockParserTest.cs(3 hunks)global.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (19)
- global.json
- Maple2.File.Tests/AdditionalEffectParserTest.cs
- Maple2.File.Tests/FunctionCubeParserTest.cs
- Maple2.File.Tests/XBlockParserTest.cs
- Maple2.File.Parser/Xml/Map/Map.cs
- Maple2.File.Tests/ScriptParserTest.cs
- Maple2.File.Parser/Xml/Script/NpcScript.cs
- Maple2.File.Tests/SkillParserTest.cs
- Maple2.File.Parser/ScriptParser.cs
- Maple2.File.Tests/NifParserTest.cs
- Maple2.File.Parser/RidingParser.cs
- Maple2.File.Parser/QuestParser.cs
- Maple2.File.Parser/ItemOptionParser.cs
- Maple2.File.Tests/ItemOptionParserTest.cs
- Maple2.File.Parser/SkillParser.cs
- Maple2.File.Parser/NpcParser.cs
- Maple2.File.Parser/ItemParser.cs
- Maple2.File.Tests/NpcParserTest.cs
- Maple2.File.Parser/MapParser.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (18)
Maple2.File.Parser/PetParser.cs (1)
40-40: Improves null safetyUsing
GetValueOrDefault(petId) ?? string.Emptyensures that when a pet name isn't found in the dictionary, an empty string is returned rather than null. This aligns with the pattern across the PR of using null-coalescing operators to provide safer string handling.Maple2.File.Tests/RidingParserTest.cs (2)
12-12: Good initialization patternExplicitly loading the filter for the "NA" region before creating the parser ensures consistent test behavior and aligns with the changes across other test classes in this PR.
Also applies to: 27-27
40-58: Comprehensive KR data test coverageThe new test method for Korean riding data follows the same verification pattern as the NA tests while accounting for KR-specific data structures. The validation of passengers within riding objects and the assertion of the expected total count (615) provide good test coverage.
Maple2.File.Tests/MapParserTest.cs (3)
12-12: Improved test structure and validationAdding the filter initialization and count validation ensures that the test is properly configured for the NA region and verifies the exact number of maps parsed (1200), providing stronger test assertions.
Also applies to: 20-27
30-48: Well-structured KR map parser testThis test follows the same verification pattern as the NA counterpart while accounting for the different dataset size in the KR version. The assertion of 1299 maps provides a clear validation point for the KR parsing functionality.
50-66: Good coverage for map name parsingThe addition of tests for
ParseMapNamesfor both NA and KR locales ensures complete coverage of the map parsing functionality. The different count assertions (1152 for NA, 1282 for KR) properly validate the region-specific behavior.Maple2.File.Tests/TestUtils.cs (2)
54-96: Well-structured utility class for KR testingThe new
TestUtilsKrclass provides a clean, centralized way to access Korean data files for testing, following the same pattern as the existingTestUtilsclass. The method replication ensures consistent behavior while maintaining a clear separation between NA and KR test environments.While there is duplication between the classes, the separation provides clarity for region-specific testing purposes.
66-68: Appropriate environment validationThe check for the KMS2_DATA_FOLDER environment variable with a clear error message ensures that tests will fail early with a meaningful error if the required test data isn't available.
Maple2.File.Tests/ItemParserTest.cs (5)
12-12: Filter.Load now explicitly includes TestUtils.XmlReaderYou've updated the
Filter.Loadcall to explicitly passTestUtils.XmlReader. This makes the dependency clearer and more explicit.
20-28: Improved test structure with count verificationGood addition of count tracking and verification with
Assert.AreEqual(35309, count). This ensures the parser is processing the expected number of items, making the test more robust.
31-49: Well-structured KR locale test methodThe new
TestItemParserKrmethod follows good testing practices:
- It loads the appropriate KR filter
- Uses the Korean XML reader
- Verifies each parsed item
- Ensures the total count matches expectations
This provides good test coverage for the Korean locale implementation.
51-58: Good addition of ItemNames test for NA localeThis test correctly validates that the
ItemNames()method returns the expected number of entries for the NA locale.
60-67: Good addition of ItemNames test for KR localeThis test complements the NA locale test by validating the
ItemNames()method against Korean data.Maple2.File.Tests/QuestParserTest.cs (5)
12-12: Filter.Load now explicitly includes TestUtils.XmlReaderYou've updated the
Filter.Loadcall to explicitly passTestUtils.XmlReader. This makes the dependency clearer and more explicit.
20-27: Improved test structure with count verificationGood addition of count tracking and verification with
Assert.AreEqual(4903, count). This ensures the parser is processing the expected number of quests, making the test more robust.
30-48: Well-implemented KR quest parser testThe new
TestQuestParserKrmethod provides good test coverage for Korean quest data:
- It loads the appropriate KR filter
- Uses the Korean XML reader
- Uses the specialized
ParseKr()method that returnsQuestDataKRobjects- Verifies each parsed quest
- Ensures the total count matches expectations (6961)
This is a good addition for testing the new Korean locale support.
50-63: Good test coverage for quest descriptionsThe new
TestQuestDescriptionParsermethod adds test coverage for a previously untested feature - quest descriptions. It correctly:
- Validates that each quest description has a positive ID and non-null name
- Verifies the expected count of descriptions
This increases test coverage and confidence in the quest description parsing functionality.
65-78: Good test coverage for KR quest descriptionsThis test complements the NA quest description test by validating the
ParseQuestDescriptions()method against Korean data. The implementation is consistent with the other test methods and properly verifies the expected count (5793).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Maple2.File.Tests/TableParserKRTest.cs (2)
19-27: Enhance test assertions for most test methodsMost test methods only iterate over parsing results without making assertions about the expected data. While this verifies no exceptions are thrown, it doesn't validate that the data is correct or complete.
Consider enhancing at least some of the critical test methods with assertions that verify:
- The expected number of items is parsed
- Key objects contain expected values
- Required properties are not null or empty
For example, similar to how
TestParseSetItemOptionKR(lines 362-377) andTestIndividualItemDropKR(lines 386-393) include count assertions.Here's a template for implementing improved assertions:
[TestMethod] public void TestExampleImprovedMethod() { int count = 0; foreach ((int id, var item) in _parser.ParseExample()) { count++; // Add assertions for important items if (id == knownItemId) { Assert.IsNotNull(item.ImportantProperty); Assert.AreEqual(expectedValue, item.SomeProperty); } } // Verify total count is as expected Assert.IsTrue(count > 0, "No items were parsed"); // If the exact count is stable, assert it // Assert.AreEqual(expectedCount, count); }Also applies to: 29-43, 45-113, 115-141, 143-190, 192-214, 216-256, 293-349, 355-361, 379-385, 395-428, 430-442, 444-463, 465-484, 486-506, 507-554, 577-613, 622-656
286-290: Remove obsolete assertions from TestParseJobTableKRThe commented‑out checks for
learn,startInvenItemandrewardrefer to fields that don't exist onJobTableKRand should be removed.- // Assert.IsTrue(job[0].learn.Count > 0); - // // Ensure the right amount of items are parsed - // Assert.AreEqual(job[0].startInvenItem.item.Count, itemCount); - // Assert.AreEqual(job[0].reward.item.Count, itemCount);
🧹 Nitpick comments (2)
Maple2.File.Tests/TableParserKRTest.cs (2)
9-9: Consider standardizing class naming styleThe class is named
TableParserKrTestbut elsewhere in the code "KR" is used (e.g.,ParseJobTableKR()). Consider changing toTableParserKRTestfor consistency.-public class TableParserKrTest { +public class TableParserKRTest {
19-27: Standardize method name suffix casingTest method names are inconsistent in their suffix usage. Some use "Kr" (e.g.,
TestParseScrollKr) while others use "KR" (e.g.,TestGuildExpKR). It would be better to standardize on one form, preferably "KR" to match the parsing method names.For example:
-public void TestColorPaletteParserKr() { +public void TestColorPaletteParserKR() {And similar changes throughout the file.
Also applies to: 45-85, 115-141, 192-214, 293-349
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Maple2.File.Parser/Enum/ConditionType.cs(1 hunks)Maple2.File.Tests/TableParserKRTest.cs(1 hunks)Maple2.File.Tests/TableParserTest.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Maple2.File.Parser/Enum/ConditionType.cs
- Maple2.File.Tests/TableParserTest.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: format
🔇 Additional comments (5)
Maple2.File.Tests/TableParserKRTest.cs (5)
164-169: Test is now correctly marked as IgnoredThe test method
TestGuildExpKris appropriately marked with[Ignore]and the method call is correctly usingParseGuildExp().
259-291: LGTM! Job table test has proper assertionsThe test properly verifies job groups, features filtering, and skill counts. Good job on grouping jobs by job group ID to facilitate efficient testing and verification.
362-377: Well-implemented test with specific assertionsThis test for
TestParseSetItemOptionKris well implemented with specific assertions for a known item ID (15123101) and verifies the total count of items parsed. This approach provides strong validation that the parser is working correctly.
386-393: Well-implemented test with count validationThis test correctly validates the total count of individual item drops, providing confidence that all expected data is being parsed.
563-575: Properly documented ignored testsTests are appropriately marked with
[Ignore]attributes with clear comments indicating they're for features removed from KMS2 (Korean MapleStory 2). This is good practice to explain why tests are skipped.Also applies to: 615-620
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
Maple2.File.Tests/TableParserKRTest.cs (2)
282-286: Remove obsolete assertions from TestParseJobTableKrThe commented-out checks for
learn,startInvenItemandrewardrefer to fields that don't exist onJobTableKRand should be removed. If you need to verify KR-specific data, replace them with assertions againststartItems(e.g.Assert.AreEqual(job[0].startItems.Length, itemCount)).
18-651: 🛠️ Refactor suggestionEnhance test assertions for most test methods
Most test methods only iterate over parsing results without making assertions about the expected data. While this verifies no exceptions are thrown, it doesn't validate that the data is correct or complete.
Consider enhancing at least some of the critical test methods with assertions that verify:
- The expected number of items is parsed
- Key objects contain expected values
- Required properties are not null or empty
For example, similar to how
TestParseSetItemOptionKr(lines 359-373) andTestIndividualItemDropKr(lines 383-389) include count assertions.Here's a template for implementing improved assertions:
[TestMethod] public void TestExampleImprovedMethod() { int count = 0; foreach ((int id, var item) in _parser.ParseExample()) { count++; // Add assertions for important items if (id == knownItemId) { Assert.IsNotNull(item.ImportantProperty); Assert.AreEqual(expectedValue, item.SomeProperty); } } // Verify total count is as expected Assert.IsTrue(count > 0, "No items were parsed"); // If the exact count is stable, assert it // Assert.AreEqual(expectedCount, count); }Maple2.File.Parser/TableParser.cs (1)
755-765:⚠️ Potential issueIncorrect XML file used in
ParseSetItemOptionKR().You're using "table/setiteminfo.xml" but deserializing with
setItemOptionKRSerializerintoSetItemOptionRootKR. Based on previous comments, since KR doesn't have this feature, consider returning an empty collection instead.The variable
strinteis also defined but never used.public IEnumerable<(int Id, SetItemOptionKR Option)> ParseSetItemOptionKR() { - IO.Crypto.Common.PackFileEntry entry = xmlReader.GetEntry("table/setiteminfo.xml"); - var strinte = xmlReader.GetString(entry); - XmlReader reader = xmlReader.GetXmlReader(entry); - var data = setItemOptionKRSerializer.Deserialize(reader) as SetItemOptionRootKR; - Debug.Assert(data != null); - - foreach (SetItemOptionKR option in data.option) { - yield return (option.id, option); - } + // KR doesn't have this XML because color palettes locked behind achievements wasn't a KMS2 thing + return Enumerable.Empty<(int Id, SetItemOptionKR Option)>(); }
🧹 Nitpick comments (2)
Maple2.File.Parser/TableParser.cs (2)
964-965: Remove extra blank lines.There are two consecutive empty lines that should be removed to maintain consistent code formatting.
public IEnumerable<(int ItemId, ShopFurnishing UgcItem)> ParseFurnishingShopUgcAll() { string xml = Sanitizer.RemoveEmpty(xmlReader.GetString(xmlReader.GetEntry($"table/{locale}/shop_ugcall.xml"))); xml = Sanitizer.RemoveUtf8Bom(xml); - var reader = XmlReader.Create(new StringReader(xml));
259-264: Consistent locale-based file path handling across the codebase.You've done a good job implementing locale-aware file path selection throughout the parser. This will make it easier to support both default and Korean locales.
For consistency, consider using a helper method for path construction to reduce code duplication.
// Add a private helper method: private string GetLocalizedPath(string basePath, string fileName, bool useLocaleSubfolder = true) { if (useLocaleSubfolder) { return $"{basePath}/{locale}/{fileName}"; } else if (locale == "kr") { return $"{basePath}/{fileName}"; } else { return $"{basePath}/na/{fileName}"; } } // Then use it like: // string filePath = GetLocalizedPath("table", "dungeonroom.xml", false); // Or: // string filePath = GetLocalizedPath("table", "dungeonconfig.xml");Also applies to: 302-304, 313-314, 323-324, 531-535, 546-547, 557-558, 914-915, 928-929, 967-969, 980-981, 991-992, 1069-1070, 1102-1103, 1135-1136, 1231-1232, 1297-1298, 1308-1309, 1374-1375
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Maple2.File.Parser/TableParser.cs(33 hunks)Maple2.File.Tests/TableParserKRTest.cs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Maple2.File.Tests/TableParserKRTest.cs (6)
Maple2.File.Parser/TableParser.cs (2)
TableParser(13-1383)TableParser(105-204)Maple2.File.Parser/Tools/Filter.cs (1)
Filter(9-47)Maple2.File.Tests/TestUtils.cs (2)
TestUtilsKr(55-96)TestUtilsKr(63-87)Maple2.File.Parser/SkillParser.cs (1)
Dictionary(53-66)Maple2.File.Parser/Xml/Table/JobTable.cs (1)
JobTableKR(76-88)Maple2.File.Parser/Xml/Table/Feature.cs (1)
Feature(11-19)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: format
🔇 Additional comments (14)
Maple2.File.Tests/TableParserKRTest.cs (8)
1-16: Class initialization looks good!The class initialization correctly sets up the TableParser with the KR locale and Live environment, which is essential for testing Korean-specific data parsing functionality.
160-165: Correctly marked as Ignored with appropriate commentGood use of the
[Ignore]attribute with a clear comment explaining why this test is skipped (feature removed from KMS2).
162-163: Fixed method call in TestGuildExpKrThis method correctly calls
ParseGuildExp()which fixes the issue identified in a previous review where it incorrectly calledParseGuildEvent().
255-287: Good implementation with proper assertionsThe
TestParseJobTableKrmethod includes proper grouping of parsed job tables and meaningful assertions about the expected counts and feature filtering, which makes it a more thorough test than most others in this file.
359-373: Good assertions in TestParseSetItemOptionKrThis test method includes specific assertions verifying both particular item properties and the total count of parsed items, which makes it a more valuable test than those that just iterate through results.
383-389: Good count assertion in TestIndividualItemDropKrThis test method verifies the total count of parsed drops, which is a good validation check.
441-445: TestAdventureLevelAbilityKr is properly implementedThis test method correctly uses direct parsing without redundant operations that were flagged in previous reviews.
559-571: Correctly marked tests as IgnoredThe
TestSurvivalLevelKrandTestSurvivalLevelRewardKrmethods are properly marked with[Ignore]attributes and include comments explaining they test features removed from KMS2.Maple2.File.Parser/TableParser.cs (6)
354-354: Adding null check for fish names.This change is necessary since
GetValueOrDefaultreturnsstring?and we need to ensure a non-null value when it doesn't exist in the dictionary.
493-493: Good addition of null handling for name lookups.Adding the null coalescing operator (
??) to ensure that missing names default to empty string rather than null is a good defensive programming practice.Also applies to: 741-741, 779-779
797-813: The Korean individual item drop parser looks well implemented.The parser properly handles the Korean XML format which has a different structure than the default one. The sanitization steps (removing empty content, sanitizing booleans, and removing UTF-8 BOM) ensure proper parsing of the Korean data.
103-104: Good implementation of locale-based file path selection.Adding the
localefield and initializing it fromFeatureLocaleFilter.Locale.ToLower()enables dynamic selection of XML file paths based on the current locale setting. This is a good approach for supporting multiple locales.Also applies to: 195-196
618-627: Good implementation of the Korean job table parser.The
ParseJobTableKRmethod follows the same pattern as other parsers and correctly uses thejobKRSerializerto deserialize the Korean job data.
49-50: Good addition of Korean-specific serializers.Adding dedicated serializers for Korean data types (
jobKRSerializer,setItemOptionKRSerializer, andindividualItemDropKRSerializer) is a good approach to handle the different XML schemas between locales.Also applies to: 63-64, 66-67, 141-142, 155-156, 158-159
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
Maple2.File.Parser/TableParser.cs (1)
783-797:⚠️ Potential issueCode indentation causing formatting issues
Several pipeline failures indicate whitespace formatting issues in this section. The nested anonymous object creation has incorrect indentation.
Fix the indentation to resolve formatting errors:
public IEnumerable<(int Id, IDictionary<byte, List<IndividualItemDrop>>)> ParseIndividualItemDrop() { string xml = Sanitizer.RemoveEmpty(xmlReader.GetString(xmlReader.GetEntry("table/individualitemdrop.xml"))); xml = Sanitizer.SanitizeBool(xml); var reader = XmlReader.Create(new StringReader(xml)); var data = individualItemDropSerializer.Deserialize(reader) as IndividualItemDropRoot; Debug.Assert(data != null); - var groups = data.individualDropBox.GroupBy(individualItemDrop => new { - individualItemDrop.individualDropBoxID, - individualItemDrop.dropGroup - }) + var groups = data.individualDropBox.GroupBy(individualItemDrop => new { + individualItemDrop.individualDropBoxID, + individualItemDrop.dropGroup + }) .ToDictionary(group => group.Key, group => group.ToList()); foreach (var group in groups) { - yield return (group.Key.individualDropBoxID, group.Value.GroupBy(drop => drop.dropGroup).ToDictionary(drop => drop.Key, drop => drop.ToList())); + yield return (group.Key.individualDropBoxID, + group.Value.GroupBy(drop => drop.dropGroup) + .ToDictionary(drop => drop.Key, drop => drop.ToList())); } }Apply the same indentation fix to similar blocks at lines 823-830, 839-846, 855-862, 871-878, 887-894, 903-910, 919-926, 935-942, and 951-958.
🧰 Tools
🪛 GitHub Actions: Format
[error] 790-790: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 791-791: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 792-792: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
♻️ Duplicate comments (5)
Maple2.File.Parser/TableParser.cs (3)
755-765:⚠️ Potential issueIncorrect XML file used in
ParseSetItemOptionKR()You deserialize
SetItemOptionRootKRbut open"table/setiteminfo.xml"instead of the expected"table/setitemoption.xml"(or a locale‑specific variant).
This will deserialize the wrong schema and likely fail at runtime.-IO.Crypto.Common.PackFileEntry entry = xmlReader.GetEntry("table/setiteminfo.xml"); +IO.Crypto.Common.PackFileEntry entry = xmlReader.GetEntry($"table/{locale}/setitemoption.xml");
strinteis also unused and can be removed.
755-765:⚠️ Potential issueIncorrect XML file used in
ParseSetItemOptionKR()You're deserializing
SetItemOptionRootKRbut opening"table/setiteminfo.xml"instead of the expected"table/setitemoption.xml"(or a locale-specific variant). Also, thestrintevariable is unused.-IO.Crypto.Common.PackFileEntry entry = xmlReader.GetEntry("table/setiteminfo.xml"); -var strinte = xmlReader.GetString(entry); +IO.Crypto.Common.PackFileEntry entry = xmlReader.GetEntry($"table/{locale}/setitemoption.xml"); XmlReader reader = xmlReader.GetXmlReader(entry); var data = setItemOptionKRSerializer.Deserialize(reader) as SetItemOptionRootKR;
755-765:⚠️ Potential issueIncorrect XML file used in
ParseSetItemOptionKR()You deserialize
SetItemOptionRootKRbut open"table/setiteminfo.xml"instead of the expected"table/setitemoption.xml".-IO.Crypto.Common.PackFileEntry entry = xmlReader.GetEntry("table/setiteminfo.xml"); +IO.Crypto.Common.PackFileEntry entry = xmlReader.GetEntry($"table/{locale}/setitemoption.xml");Also,
strinteis unused and can be removed:IO.Crypto.Common.PackFileEntry entry = xmlReader.GetEntry("table/setiteminfo.xml"); -var strinte = xmlReader.GetString(entry); XmlReader reader = xmlReader.GetXmlReader(entry);Maple2.File.Parser/QuestParser.cs (2)
1-8:⚠️ Potential issueMissing core
usingdirectives breaks compilationThe file uses types from
System,System.Collections.Generic, andSystem.IOnamespaces, but these are not imported. The code will fail to compile without these directives.using System.Diagnostics; +using System; +using System.Collections.Generic; +using System.IO; using System.Xml; using System.Xml.Serialization; using M2dXmlGenerator;
57-75:⚠️ Potential issueUse dictionary indexer instead of Add to prevent exceptions
Using
questNames.Add()will throw an exception if a duplicate key is encountered. This is likely to happen with quest IDs across multiple XML files.foreach (QuestDescription description in root.quest) { - questNames.Add(description.questID, description.name); + questNames[description.questID] = description.name; }
🧹 Nitpick comments (6)
Maple2.File.Parser/TableParser.cs (4)
808-813: Improve method chaining readabilityThe LINQ chain is quite complex and could be formatted more clearly to improve readability.
-yield return (dropBox.DropBoxID, dropBox.Groups - .GroupBy(g => (byte) g.DropGroupID) - .ToDictionary( - g => g.Key, - g => g.SelectMany(grp => grp.Items).ToList() - )); +yield return ( + dropBox.DropBoxID, + dropBox.Groups + .GroupBy(g => (byte) g.DropGroupID) + .ToDictionary( + g => g.Key, + g => g.SelectMany(grp => grp.Items).ToList() + ) +);
987-987: Redundant empty lineThere's an unnecessary empty line after the UTF-8 BOM removal.
xml = Sanitizer.RemoveUtf8Bom(xml); - var reader = XmlReader.Create(new StringReader(xml));
802-803: Consider adding comment explaining UTF-8 BOM removalThe code removes UTF-8 BOM from the XML string, but it's not clear why this is necessary. Adding a comment would help explain the purpose.
xml = Sanitizer.SanitizeBool(xml); +// Remove UTF-8 BOM to ensure proper XML parsing of Korean characters xml = Sanitizer.RemoveUtf8Bom(xml);
103-196: Locale initialization is good, but consider uppercase comparisonThe locale initialization is well-implemented, but consider using case-insensitive comparison for better reliability.
-locale = FeatureLocaleFilter.Locale.ToLower(); +locale = FeatureLocaleFilter.Locale.ToLowerInvariant();Maple2.File.Parser/QuestParser.cs (2)
25-40: Consider more robust error handling for file accessThe
Parse()method now correctly handles quest data parsing, but it doesn't have robust error handling for cases where XML entries might not exist or could be malformed.Consider adding try-catch blocks to handle potential IO and XML deserialization exceptions:
public IEnumerable<(int Id, string Name, QuestData Data)> Parse() { Dictionary<int, string> questNames = ParseQuestDescriptions(); foreach (PackFileEntry entry in xmlReader.Files.Where(entry => entry.Name.StartsWith("quest/"))) { - var reader = XmlReader.Create(new StringReader(Sanitizer.SanitizeQuest(xmlReader.GetString(entry)))); - - var root = questSerializer.Deserialize(reader) as QuestDataRootRoot; - Debug.Assert(root != null); + try { + var reader = XmlReader.Create(new StringReader(Sanitizer.SanitizeQuest(xmlReader.GetString(entry)))); + + var root = questSerializer.Deserialize(reader) as QuestDataRootRoot; + if (root == null) continue; + + QuestData? data = root.environment?.quest; + if (data == null) continue; + + int questId = int.Parse(Path.GetFileNameWithoutExtension(entry.Name)); + yield return (questId, questNames.GetValueOrDefault(questId, string.Empty), data); + } catch (Exception ex) { + Debug.WriteLine($"Error parsing quest file {entry.Name}: {ex.Message}"); + continue; + } - - QuestData? data = root.environment?.quest; - if (data == null) continue; - - int questId = int.Parse(Path.GetFileNameWithoutExtension(entry.Name)); - yield return (questId, questNames.GetValueOrDefault(questId, string.Empty), data); } }
42-55: Similar error handling needed for KR parserThe
ParseKr()method has similar issues with error handling as theParse()method.Consider using a try-catch block to handle potential exceptions:
public IEnumerable<(int Id, string Name, QuestDataKR Data)> ParseKr() { Dictionary<int, string> questNames = ParseQuestDescriptions(); PackFileEntry? entry = xmlReader.GetEntry("questdata.xml"); Debug.Assert(entry != null, "questdata.xml not found"); - var reader = XmlReader.Create(new StringReader(Sanitizer.SanitizeQuest(xmlReader.GetString(entry)))); - - var root = questSerializer.Deserialize(reader) as QuestDataRootKR; - Debug.Assert(root != null); - - foreach (QuestDataKR data in root.quests) { - yield return (data.id, questNames.GetValueOrDefault(data.id, string.Empty), data); + try { + var reader = XmlReader.Create(new StringReader(Sanitizer.SanitizeQuest(xmlReader.GetString(entry)))); + + var root = questSerializer.Deserialize(reader) as QuestDataRootKR; + if (root == null) yield break; + + foreach (QuestDataKR data in root.quests) { + yield return (data.id, questNames.GetValueOrDefault(data.id, string.Empty), data); + } + } catch (Exception ex) { + Debug.WriteLine($"Error parsing KR quest data: {ex.Message}"); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Maple2.File.Parser/ItemParser.cs(2 hunks)Maple2.File.Parser/MapParser.cs(2 hunks)Maple2.File.Parser/NpcParser.cs(2 hunks)Maple2.File.Parser/PetParser.cs(1 hunks)Maple2.File.Parser/QuestParser.cs(3 hunks)Maple2.File.Parser/SkillParser.cs(2 hunks)Maple2.File.Parser/TableParser.cs(38 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- Maple2.File.Parser/PetParser.cs
- Maple2.File.Parser/NpcParser.cs
- Maple2.File.Parser/ItemParser.cs
- Maple2.File.Parser/SkillParser.cs
- Maple2.File.Parser/MapParser.cs
🧰 Additional context used
🧬 Code Graph Analysis (1)
Maple2.File.Parser/QuestParser.cs (5)
Maple2.File.Generator/Resource/FeatureLocaleFilter.cs (1)
FeatureLocaleFilter(7-76)Maple2.File.Parser/PetParser.cs (2)
IEnumerable(25-42)IEnumerable(44-50)Maple2.File.Parser/Xml/Quest/Quest.cs (2)
QuestData(17-54)QuestDataKR(62-64)Maple2.File.IO/M2dReader.cs (3)
PackFileEntry(39-41)XmlReader(43-45)GetString(64-67)Maple2.File.Parser/Tools/Sanitizer.cs (2)
Sanitizer(5-105)SanitizeQuest(61-65)
🪛 GitHub Actions: Format
Maple2.File.Parser/TableParser.cs
[error] 790-790: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 791-791: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 792-792: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 824-824: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 825-825: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 826-826: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 840-840: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 841-841: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 842-842: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 856-856: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 857-857: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 858-858: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 872-872: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 873-873: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 874-874: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 888-888: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 889-889: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 890-890: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 904-904: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 905-905: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 906-906: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 920-920: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 921-921: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 922-922: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 936-936: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 937-937: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 938-938: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 952-952: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 953-953: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
[error] 954-954: dotnet-format whitespace check failed: Fix whitespace formatting. Delete 4 characters.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (15)
Maple2.File.Parser/TableParser.cs (14)
103-103: LGTM: Locale field for consistent path resolutionGood approach adding a locale field initialized from FeatureLocaleFilter.Locale to standardize locale-aware file path resolution throughout the class.
Also applies to: 195-195
354-354: LGTM: Safer dictionary lookups with GetValueOrDefaultGood improvement using
GetValueOrDefaultwith empty string fallback for dictionary lookups. This prevents null reference exceptions when keys aren't found.Also applies to: 493-493, 741-741, 779-779
618-627: LGTM: Well-implemented Korean job table parserThe new ParseJobTableKR method correctly handles parsing Korean-specific job data.
799-814: LGTM: Well-structured KR-specific item drop parserThe ParseIndividualItemDropKR method properly handles Korean-specific item drop data, including sanitization of XML content before deserialization.
987-988: LGTM: UTF-8 BOM handlingGood addition of UTF-8 BOM removal for XML deserialization. This helps prevent parsing errors with Korean character encoding.
259-262: LGTM: Locale-specific file path selectionGood implementation of conditional file path selection based on locale. This pattern is consistently applied throughout the file for various resource types.
49-64: Good serializer additions for Korean data formatsThe new serializers for Korean-specific data types are correctly implemented.
258-270: Good locale-specific file path handlingThe conditional selection of file paths based on locale is well-implemented.
302-310: Consistent locale path formattingThe string interpolation for locale paths is concise and readable.
354-355: Good null-safe handling of dictionary lookupUsing
GetValueOrDefaultwith an empty string fallback ensures that no null values will be returned.
618-627: Well-structured KR job table parserThe implementation of
ParseJobTableKR()is clean and follows the same pattern as other parsers.
799-815: Well-structured KR item drop parserThe implementation of
ParseIndividualItemDropKR()is well organized and properly handles the Korean data format. The use ofSanitizer.RemoveUtf8Bom(xml)is a good addition to ensure proper handling of encoded files.
985-996: Good BOM handling for UGC XMLAdding UTF-8 BOM removal for UGC XML files is important for proper parsing.
1088-1096: Consistent use of locale for UGC design pathsThe locale-specific path handling is consistent with other methods.
Maple2.File.Parser/QuestParser.cs (1)
18-23: Looks good - locale-aware serializer implementationThe constructor now correctly selects the appropriate serializer type based on the locale, allowing for proper deserialization of both standard and Korean quest data formats.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Maple2.File.Parser/TableParser.cs (2)
790-796: Fixed indentation in anonymous object declarations.The indentation has been fixed in the GroupBy expression, which should resolve the formatting pipeline errors.
755-765:⚠️ Potential issueFix incorrect XML file path in
ParseSetItemOptionKR().The method is deserializing
SetItemOptionRootKRbut is using the wrong XML file path ("table/setiteminfo.xml" instead of "table/setitemoption.xml" or a locale-specific variant). This would result in deserializing the wrong schema.Additionally, the
strintevariable at line 757 is declared but never used.Apply this fix:
-IO.Crypto.Common.PackFileEntry entry = xmlReader.GetEntry("table/setiteminfo.xml"); -var strinte = xmlReader.GetString(entry); +IO.Crypto.Common.PackFileEntry entry = xmlReader.GetEntry($"table/{locale}/setitemoption.xml");
🧹 Nitpick comments (3)
Maple2.File.Parser/TableParser.cs (3)
258-262: Inconsistent locale path handling.Some methods use explicit if-checks for locale, while others use string interpolation. Consider standardizing on one approach for better maintainability.
-string filename = "table/na/dungeonroom.xml"; -if (locale == "kr") { - filename = "table/dungeonroom.xml"; -} -XmlReader reader = xmlReader.GetXmlReader(xmlReader.GetEntry(filename)); +string prefix = locale == "kr" ? "" : "na/"; +XmlReader reader = xmlReader.GetXmlReader(xmlReader.GetEntry($"table/{prefix}dungeonroom.xml"));Or alternatively, use the direct string interpolation approach used in other methods:
-string filename = "table/na/dungeonroom.xml"; -if (locale == "kr") { - filename = "table/dungeonroom.xml"; -} -XmlReader reader = xmlReader.GetXmlReader(xmlReader.GetEntry(filename)); +XmlReader reader = xmlReader.GetXmlReader(xmlReader.GetEntry($"table/{(locale == "kr" ? "" : "na/")}dungeonroom.xml"));
530-535: Duplicate code pattern.This pattern of locale-specific file selection is repeated throughout the code. Consider extracting it to a helper method to improve maintainability.
+private string GetLocaleFilePath(string baseFilePath, string defaultLocale = "na") { + return locale == "kr" ? $"table/{baseFilePath}" : $"table/{defaultLocale}/{baseFilePath}"; +} // Then use it like this: -string filename = "table/na/itemextraction.xml"; -if (locale == "kr") { - filename = "table/itemextraction.xml"; -} -string xml = Sanitizer.RemoveEmpty(xmlReader.GetString(xmlReader.GetEntry(filename))); +string xml = Sanitizer.RemoveEmpty(xmlReader.GetString(xmlReader.GetEntry(GetLocaleFilePath("itemextraction.xml"))));
302-303: Consider error handling for missing locale-specific files.The code assumes that every locale will have its own dungeon config file. Consider adding error handling in case a file doesn't exist for a specific locale.
-XmlReader reader = xmlReader.GetXmlReader(xmlReader.GetEntry($"table/{locale}/dungeonconfig.xml")); +IO.Crypto.Common.PackFileEntry entry; +try { + entry = xmlReader.GetEntry($"table/{locale}/dungeonconfig.xml"); +} catch (Exception) { + // Fallback to default locale + entry = xmlReader.GetEntry("table/na/dungeonconfig.xml"); +} +XmlReader reader = xmlReader.GetXmlReader(entry);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Maple2.File.Parser/TableParser.cs(38 hunks)
🔇 Additional comments (7)
Maple2.File.Parser/TableParser.cs (7)
799-803: Good handling of UTF-8 BOM removal.The addition of
Sanitizer.RemoveUtf8Bom(xml)is good practice for handling Korean text files that might include a Byte Order Mark.
354-354: Good null-safety handling.Using
GetValueOrDefaultwith a default empty string is a good practice to handle missing fish names.
618-627: New Korean job table parsing functionality.The new parsing method for Korean job tables is well-implemented. Consider adding unit tests to ensure the functionality works as expected.
Is there a corresponding unit test for this new functionality? It would be important to verify that the XML deserialization works correctly with Korean data structures.
986-988: Important UTF-8 BOM removal for UGC content.The addition of UTF-8 BOM removal is particularly important for user-generated content files, as they might come from different sources with different text encodings.
807-814: Different structure for Korean drop data.The Korean implementation has a different structure for handling drop data compared to the non-Korean version. This is a good design choice when dealing with different data schemas.
103-104: Locale field addition.Adding the locale field is a good design choice for making the parser locale-aware.
195-196: Locale initialization.The locale is properly initialized from the global FeatureLocaleFilter. This ensures consistency across the application.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
global.jsonfor .NET SDK management.