Part 2 of 3 for #123 - Fix sealed & visibility issues part2#125
Merged
danipen merged 8 commits intodanipen:masterfrom Mar 15, 2026
Merged
Conversation
Refactored TMModel.cs to implement the recommended Dispose pattern: - Replaced original Dispose with public Dispose and protected virtual Dispose(bool). - Added detailed XML documentation for disposal methods. - Ensured managed resources are disposed only once via Interlocked.CompareExchange inside the isDisposing branch. - Moved resource cleanup logic to managed disposal section. - Suppressed finalization and enabled safe extension for derived classes. - Structure allows for future unmanaged resource handling.
Strengthen encapsulation and immutability Several classes changed from public to internal and sealed, with fields and methods updated to internal and readonly where appropriate. GrammarNames made static; SupportedGrammars is now readonly. These changes improve code safety and clarify usage boundaries.
Introduced CustomBenchmarksConfig to display memory usage in kilobytes for more precise measurement in benchmark summaries. Applied this config to BigFileTokenizationBenchmark via attribute. Cleaned up using statements and improved formatting.
Introduced AllocatedBytesColumn for detailed memory usage reporting in benchmark results. Updated CustomBenchmarksConfig to include this column and set summary style to kilobytes and current culture. Minor formatting and console message improvements.
Refactored variable naming for clarity. Replaced exception-based parsing with TryParse for integers and floats, providing more accurate error messages. Corrected grammar in integer error message.
Added InternalsVisibleTo for DynamicProxyGenAssembly2 in AssemblyInfo.cs, allowing internal members to be accessed by mocking frameworks such as Moq during unit testing. Existing access for TextMateSharp.Tests remains unchanged.
Apply signature parity with tm4e upstream per https://github.com/eclipse-tm4e/tm4e/tree/main/org.eclipse.tm4e.core/src/main/java/org/eclipse/tm4e/core/internal/rule Restrict visibility and improve encapsulation Refactored multiple classes to be internal and/or sealed, limiting access to within the assembly. Updated constructors, properties, and methods for better encapsulation and immutability. Renamed methods to follow C# conventions. Made static fields in RuleId readonly. Converted ThemeReader to a static class. Removed ICompilePatternsResult.cs and merged its contents. These changes clarify API boundaries and enhance maintainability.
udlose
changed the title
Owner
|
Overall LGTM. Let's see if Copilot finds anything interesting. Sorry for the delay!! |
There was a problem hiding this comment.
Pull request overview
This PR (part 2/3 of #123) tightens encapsulation/immutability across internal grammar/rule infrastructure, refines parsing and disposal patterns, and enhances benchmark memory reporting.
Changes:
- Introduces BenchmarkDotNet custom config/column to report allocated bytes per operation.
- Refactors many internal grammar/rule types (visibility, sealing, readonly fields) to align closer with upstream tm4e and reduce mutability.
- Improves parsing/disposal correctness (plist numeric parsing refactor; TMModel dispose pattern refactor) and adds
InternalsVisibleTofor proxy-based mocking.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/TextMateSharp/Properties/AssemblyInfo.cs | Adds InternalsVisibleTo for DynamicProxyGenAssembly2 for mocking internal types. |
| src/TextMateSharp/Model/TMModel.cs | Refactors dispose pattern into Dispose(bool) with additional docs/guarding. |
| src/TextMateSharp/Internal/Themes/reader/ThemeReader.cs | Makes ThemeReader static and removes unused usings. |
| src/TextMateSharp/Internal/Rules/RuleId.cs | Makes singleton RuleId instances readonly; converts Id to get-only property. |
| src/TextMateSharp/Internal/Rules/RuleFactory.cs | Narrows CreateCaptureRule visibility to reduce API surface. |
| src/TextMateSharp/Internal/Rules/Rule.cs | Adjusts visibility/immutability (e.g., internal Id, readonly fields, protected ctor). |
| src/TextMateSharp/Internal/Rules/RegExpSourceList.cs | Seals and makes internalize core mutators; switches to field initializers. |
| src/TextMateSharp/Internal/Rules/RegExpSource.cs | Seals/internalizes; increases immutability and makes helpers static/readonly. |
| src/TextMateSharp/Internal/Rules/MatchRule.cs | Seals and internalizes ctor; makes regexp source readonly. |
| src/TextMateSharp/Internal/Rules/IncludeOnlyRule.cs | Internalizes/seals rule and narrows property visibility. |
| src/TextMateSharp/Internal/Rules/ICompilePatternsResult.cs | Removes old CompilePatternsResult definition file. |
| src/TextMateSharp/Internal/Rules/CompilePatternsResult.cs | Reintroduces CompilePatternsResult as internal sealed with internal members. |
| src/TextMateSharp/Internal/Rules/CompiledRule.cs | Seals and internalizes ctor. |
| src/TextMateSharp/Internal/Rules/CaptureRule.cs | Seals and internalizes ctor; minor whitespace cleanup. |
| src/TextMateSharp/Internal/Rules/BeginWhileRule.cs | Seals/internalizes ctor; readonly fields; renames while backref method to PascalCase. |
| src/TextMateSharp/Internal/Rules/BeginEndRule.cs | Seals/internalizes ctor; readonly fields; reduces property exposure. |
| src/TextMateSharp/Internal/Parser/PList.cs | Refactors numeric parsing and error handling; renames local var for clarity. |
| src/TextMateSharp/Internal/Matcher/IMatchesName.cs | Seals NameMatcher. |
| src/TextMateSharp/Internal/Grammars/LineTokenizer.cs | Updates call site for renamed while backref method. |
| src/TextMateSharp/Internal/Grammars/BasicScopeAttributesProvider.cs | Seals/internalizes provider; fixes regex join bug; increases immutability. |
| src/TextMateSharp/Internal/Grammars/BasicScopeAttributes.cs | Makes attributes internal sealed and narrows member visibility. |
| src/TextMateSharp/Internal/Grammars/BalancedBracketSelectors.cs | Seals and makes internal some behavioral methods; increases immutability. |
| src/TextMateSharp/Internal/Grammars/AttributedScopeStack.cs | Makes stack internal sealed and narrows member visibility. |
| src/TextMateSharp.Grammars/Properties/AssemblyInfo.cs | Adds InternalsVisibleTo for DynamicProxyGenAssembly2. |
| src/TextMateSharp.Grammars/GrammarNames.cs | Makes GrammarNames static and SupportedGrammars readonly. |
| src/TextMateSharp.Benchmarks/BigFileTokenizationBenchmark.cs | Adds custom BenchmarkDotNet config + allocation column; minor cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
95
to
+102
| else if ("real".Equals(tagName)) | ||
| { | ||
| try | ||
| { | ||
| value = float.Parse(text); | ||
| } | ||
| catch (Exception) | ||
| if (!float.TryParse(t, out float f)) | ||
| { | ||
| errors.Add(text + " is not a float"); | ||
| errors.Add(t + " is not a float"); | ||
| return; | ||
| } | ||
| value = f; |
Comment on lines
8
to
+18
| public abstract class Rule | ||
| { | ||
| public RuleId Id { get; private set; } | ||
| internal RuleId Id { get; private set; } | ||
|
|
||
| private bool _nameIsCapturing; | ||
| private string _name; | ||
| private readonly bool _nameIsCapturing; | ||
| private readonly string _name; | ||
|
|
||
| private bool _contentNameIsCapturing; | ||
| private string _contentName; | ||
| private readonly bool _contentNameIsCapturing; | ||
| private readonly string _contentName; | ||
|
|
||
| public Rule(RuleId id, string name, string contentName) | ||
| protected Rule(RuleId id, string name, string contentName) |
Comment on lines
+23
to
27
| internal void Push(RegExpSource item) | ||
| { | ||
| this._items.Add(item); | ||
| this._hasAnchors = this._hasAnchors ? this._hasAnchors : item.HasAnchor(); | ||
| } |
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
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.
Part 2 of 3 for #123
This pull request introduces several improvements to encapsulation and immutability for internal classes, adds a custom benchmark configuration for memory measurement, and refactors code for clarity and correctness. The most important changes are grouped below by theme.
Benchmark enhancements
CustomBenchmarksConfig) and a new memory allocation column (AllocatedBytesColumn) inBigFileTokenizationBenchmark.csto provide more detailed memory usage reporting during benchmarks. [1] [2]Encapsulation and immutability improvements
AttributedScopeStack,BalancedBracketSelectors,BasicScopeAttributes,BasicScopeAttributesProvider,BeginEndRule,BeginWhileRule,CaptureRule,NameMatcher, andCompilePatternsResult) from public to internal or sealed, and made fields readonly where appropriate to improve encapsulation and immutability. [1] [2] [3] [4] [5] [6] [7] [8] [9]Refactoring for clarity and correctness
PList.csto use local variabletinstead oftext, improved error handling for integer and float parsing, and simplified exception handling. [1] [2]BasicScopeAttributesProviderwhere the wrong variable was used in a regex string join, now correctly joiningreversedScopes.getWhileWithResolvedBackReferencestoGetWhileWithResolvedBackReferencesfor consistency inBeginWhileRule. [1] [2]Internal visibility for testing
InternalsVisibleToforDynamicProxyGenAssembly2inAssemblyInfo.csto enable mocking and testing of internal members.Minor improvements
GrammarNamesto a static class and its array toreadonlyfor improved safety and clarity.