Conversation
There was a problem hiding this comment.
Pull request overview
This pull request introduces a major architectural change to USFMToolsSharp by implementing a new hierarchy model for USFM markers. The changes centralize marker hierarchy management through new HierarchyNode and HierarchyDefinition classes, removing distributed AllowedContents properties from individual marker classes. The PR also updates the target framework and package versions, adds support for nested markers with '+' prefix and escaped backslashes, and refactors the marker selection logic from a switch statement to a dictionary-based approach.
Changes:
- Introduced new hierarchy model with
HierarchyNode,HierarchyDefinition, andDefaultHierarchiesclasses to centralize marker structure management - Removed
AllowedContentsproperty andTryInsertmethods from all marker classes, relocating this logic to the hierarchy system - Updated parser to support multiple hierarchies (Default, Presentation, Structure), nested markers with '+' prefix, and escaped backslashes
Reviewed changes
Copilot reviewed 90 out of 90 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| USFMToolsSharp.csproj | Updated target framework to net10.0 and version to 2.0.0, enabled nullable reference types |
| USFMToolsSharpTest.csproj | Updated target framework and test package versions |
| USFMParser.cs | Refactored marker selection to dictionary, added hierarchy definitions support, implemented nested marker and escaped backslash handling |
| HierachyNode.cs | New class providing hierarchy tree operations including child marker search, path finding, and traversal |
| HierarchyDefinition.cs | New class defining allowed children and custom insertion logic for hierarchy nodes |
| DefaultHierarchies.cs | New static class containing three predefined hierarchies (Default, Presentation, Structure) |
| USFMDocument.cs | Refactored to support multiple hierarchies with new Insert method, added backwards compatibility property |
| Marker.cs | Removed Contents list and hierarchy methods, added DefaultHierarchyNode property |
| Various Marker classes | Removed AllowedContents and TryInsert overrides, simplified implementations |
| CMarker.cs, VMarker.cs | Updated to use DefaultHierarchyNode for child marker queries |
| USFMParserTest.cs | Updated tests to use new hierarchy model, added comprehensive hierarchy tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -106,7 +120,7 @@ private static List<Marker> CleanWhitespace(List<Marker> input) | |||
| /// <returns>A List of Markers based upon the string</returns> | |||
| private List<Marker> TokenizeFromString(ReadOnlySpan<char> input) | |||
There was a problem hiding this comment.
There is the possibility that we could use a state object here instead of tracking inContent, inMarker, start, end etc. Would be interested of what you think @PurpleGuitar. It would be potentially slower, but probably more maintainable.
This pull request introduces a new hierarchy model for USFM markers, centered around the new
HierarchyNodeandHierarchyDefinitionclasses. It also removes theAllowedContentsproperty and related logic from individual marker classes, consolidating hierarchy and type-checking responsibilities. Several marker classes are updated to use the new hierarchy model for child marker queries.Key changes:
Hierarchy Model Introduction
HierarchyNodeclass to represent nodes in the USFM marker hierarchy, including methods for traversing, searching, and manipulating the hierarchy tree. (USFMToolsSharp/Models/HierachyNode.cs)HierarchyDefinitionclass to define allowed child types and insertion logic for hierarchy nodes. (USFMToolsSharp/Models/HierarchyDefinition.cs)Marker Class Refactoring
AllowedContentsproperty and related staticHashSet<Type>definitions from all marker classes, centralizing allowed child type logic in the new hierarchy model. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14]Marker Child Query Updates
CMarker) to use the newDefaultHierarchyNode.GetChildMarkers<T>()andAs<T>()for child marker queries and type casting, replacing previous direct methods. (USFMToolsSharp/Models/Markers/CMarker.cs) [1] [2]Codebase Cleanup
USFMToolsSharp/Models/Markers/FTMarker.cs)These changes collectively modernize and centralize the marker hierarchy logic, making the codebase easier to maintain and extend.