Parse Feature Tables#59
Merged
AngeloTadeucci merged 2 commits intoMS2Community:masterfrom Mar 29, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThe pull request increments the package version to 2.4.2 and adds two new public parsing methods to the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Maple2.File.Tests/TableParserTest.cs (1)
19-28: Strengthen this test beyond smoke enumeration.This currently passes even if both parsers return empty sequences. Add minimal assertions for non-empty output and non-empty keys.
✅ Example improvement
[TestMethod] public void TestFeatureParser() { - foreach ((_, _) in _parser.ParseFeatureSetting()) { - continue; - } - - foreach ((_, _) in _parser.ParseFeature()) { - continue; - } + List<(string Type, Setting Setting)> settings = _parser.ParseFeatureSetting().ToList(); + List<(string Name, Feature Feature)> features = _parser.ParseFeature().ToList(); + + Assert.IsTrue(settings.Count > 0, "Expected at least one feature setting."); + Assert.IsTrue(features.Count > 0, "Expected at least one feature."); + Assert.IsTrue(settings.All(s => !string.IsNullOrWhiteSpace(s.Type))); + Assert.IsTrue(features.All(f => !string.IsNullOrWhiteSpace(f.Name))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.File.Tests/TableParserTest.cs` around lines 19 - 28, The test TestFeatureParser currently only enumerates _parser.ParseFeatureSetting() and _parser.ParseFeature() and will pass if they return empty sequences; modify the test to assert that each parsed sequence is not empty and that each entry contains a non-empty key (e.g., assert collection.Any() and assert each key/string is not null or empty) for both _parser.ParseFeatureSetting() and _parser.ParseFeature() so the test fails when parsers return no meaningful data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Maple2.File.Parser/TableParser.cs`:
- Around line 228-233: After deserializing with
featureSettingSerializer.Deserialize into a FeatureSetting, don't rely on
Debug.Assert; explicitly check that 'data' is not null and that 'data.setting'
and 'data.feature' are not null before iterating. In the code paths around
featureSettingSerializer.Deserialize / FeatureSetting, add null checks (and
either throw a clear exception such as
InvalidOperationException/InvalidDataException or return/yield break) or treat
the collections with a null-coalescing fallback (e.g., use
Enumerable.Empty<Setting>() / Enumerable.Empty<Feature>()) so iterating over
data.setting and data.feature cannot cause a NullReferenceException in Release
builds.
---
Nitpick comments:
In `@Maple2.File.Tests/TableParserTest.cs`:
- Around line 19-28: The test TestFeatureParser currently only enumerates
_parser.ParseFeatureSetting() and _parser.ParseFeature() and will pass if they
return empty sequences; modify the test to assert that each parsed sequence is
not empty and that each entry contains a non-empty key (e.g., assert
collection.Any() and assert each key/string is not null or empty) for both
_parser.ParseFeatureSetting() and _parser.ParseFeature() so the test fails when
parsers return no meaningful data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac62983d-4fec-4763-9c82-2cdbaacc2011
📒 Files selected for processing (3)
Maple2.File.Parser/Maple2.File.Parser.csprojMaple2.File.Parser/TableParser.csMaple2.File.Tests/TableParserTest.cs
Comment on lines
+228
to
+233
| var data = featureSettingSerializer.Deserialize(reader) as FeatureSetting; | ||
| Debug.Assert(data != null); | ||
|
|
||
| foreach (Setting setting in data.setting) { | ||
| yield return (setting.type, setting); | ||
| } |
There was a problem hiding this comment.
Guard deserialization and collection nullability in release builds.
Debug.Assert on Line 229 and Line 239 won’t protect Release builds; data.setting (Line 231) and data.feature (Line 241) can still throw NullReferenceException.
🛠️ Proposed defensive fix
public IEnumerable<(string Type, Setting Setting)> ParseFeatureSetting() {
XmlReader reader = xmlReader.GetXmlReader(xmlReader.GetEntry("table/feature_setting.xml"));
- var data = featureSettingSerializer.Deserialize(reader) as FeatureSetting;
- Debug.Assert(data != null);
+ FeatureSetting data = featureSettingSerializer.Deserialize(reader) as FeatureSetting
+ ?? throw new InvalidDataException("Failed to deserialize table/feature_setting.xml");
- foreach (Setting setting in data.setting) {
+ foreach (Setting setting in data.setting ?? Enumerable.Empty<Setting>()) {
yield return (setting.type, setting);
}
}
public IEnumerable<(string Name, Feature Feature)> ParseFeature() {
XmlReader reader = xmlReader.GetXmlReader(xmlReader.GetEntry("table/feature.xml"));
- var data = featureSerializer.Deserialize(reader) as FeatureRoot;
- Debug.Assert(data != null);
+ FeatureRoot data = featureSerializer.Deserialize(reader) as FeatureRoot
+ ?? throw new InvalidDataException("Failed to deserialize table/feature.xml");
- foreach (Feature feature in data.feature) {
+ foreach (Feature feature in data.feature ?? Enumerable.Empty<Feature>()) {
yield return (feature.name, feature);
}
}Also applies to: 238-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.File.Parser/TableParser.cs` around lines 228 - 233, After
deserializing with featureSettingSerializer.Deserialize into a FeatureSetting,
don't rely on Debug.Assert; explicitly check that 'data' is not null and that
'data.setting' and 'data.feature' are not null before iterating. In the code
paths around featureSettingSerializer.Deserialize / FeatureSetting, add null
checks (and either throw a clear exception such as
InvalidOperationException/InvalidDataException or return/yield break) or treat
the collections with a null-coalescing fallback (e.g., use
Enumerable.Empty<Setting>() / Enumerable.Empty<Feature>()) so iterating over
data.setting and data.feature cannot cause a NullReferenceException in Release
builds.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Chores