feat: Implement Scriban-based HTML formatter with separated templates #22
Conversation
- Replace StringBuilder-based HTML generation with Scriban template engine - Add HtmlFormatterScriban.cs as new template-based formatter - Create separate template files in Reporting/HtmlTemplates/ directory: - main-layout.scriban: Main HTML document structure - breaking-changes.scriban: Breaking changes section - change-group.scriban: Individual change group display - configuration.scriban: Configuration details section - config-*.scriban: Configuration subsection partials - styles.css: Modern responsive CSS styling - scripts.js: Interactive JavaScript functionality - Add EmbeddedTemplateLoader.cs for resource loading - Add CustomTemplateLoader.cs for Scriban include functionality - Configure embedded resources in DotNetApiDiff.csproj - Update ReportGenerator.cs to use new Scriban formatter - Maintain single-file deployment capability with embedded resources - Remove conflicting TestTemplateLoading.cs test file Benefits: - Improved maintainability with separated template files - Better separation of concerns (logic vs presentation) - Enhanced styling with modern responsive design - Interactive UI elements (collapsible configuration) - Template reusability through include directives
Signed-off-by: jbrinkman <[email protected]>
Signed-off-by: jbrinkman <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the StringBuilder-based HTML generation with a Scriban template engine implementation, providing better maintainability and separation of concerns while preserving single-file deployment capability.
- Implements a new Scriban-based HTML formatter that uses separate template files for better organization
- Adds modern responsive CSS styling and interactive JavaScript functionality
- Maintains single-file deployment by embedding all templates as resources during build
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ReportGenerator.cs | Updates to use new HtmlFormatterScriban instead of old HtmlFormatter |
| HtmlFormatterScriban.cs | New Scriban-based HTML formatter implementation |
| HtmlFormatter.cs | Removes old StringBuilder-based HTML formatter |
| EmbeddedTemplateLoader.cs | Utility for loading embedded template resources |
| CustomTemplateLoader.cs | Scriban ITemplateLoader implementation for include directives |
| DotNetApiDiff.csproj | Adds Scriban package dependency and embedded resource configuration |
| HtmlTemplates/*.scriban | Separated template files for main layout, components, and configuration |
| HtmlTemplates/styles.css | Modern responsive CSS styling |
| HtmlTemplates/scripts.js | Interactive JavaScript functionality |
| // Get the source code of the template | ||
| return template.Page.Body.ToString(); |
There was a problem hiding this comment.
Accessing template.Page.Body.ToString() directly exposes internal implementation details of the Scriban library. This may break if the library changes its internal structure. Consider using the template's own rendering method or a more stable API.
| // Get the source code of the template | |
| return template.Page.Body.ToString(); | |
| // Render the template using the provided context | |
| return template.Render(context); |
| catch (Exception ex) | ||
| { | ||
| // Fallback to basic template if templates can't be loaded | ||
| Console.WriteLine($"Warning: Could not load templates, using fallback: {ex.Message}"); |
There was a problem hiding this comment.
Using Console.WriteLine directly for error logging is not ideal in a library. Consider using the ILogger that's available in the application or throwing a more specific exception that can be handled by the caller.
| private object PrepareConfigurationData(DotNetApiDiff.Models.Configuration.ComparisonConfiguration config) | ||
| { | ||
| return new | ||
| { | ||
| filters = new | ||
| { | ||
| include_internals = config.Filters.IncludeInternals, | ||
| include_compiler_generated = config.Filters.IncludeCompilerGenerated, | ||
| include_namespaces = config.Filters.IncludeNamespaces, | ||
| exclude_namespaces = config.Filters.ExcludeNamespaces, | ||
| include_types = config.Filters.IncludeTypes, | ||
| exclude_types = config.Filters.ExcludeTypes | ||
| }, | ||
| mappings = new | ||
| { | ||
| auto_map_same_name_types = config.Mappings.AutoMapSameNameTypes, | ||
| ignore_case = config.Mappings.IgnoreCase, | ||
| type_mappings = config.Mappings.TypeMappings.Select(kvp => new { key = kvp.Key, value = kvp.Value }), | ||
| namespace_mappings = config.Mappings.NamespaceMappings.Select(kvp => new { key = kvp.Key, value = kvp.Value }) | ||
| }, | ||
| exclusions = new | ||
| { | ||
| excluded_types = config.Exclusions.ExcludedTypes, | ||
| excluded_members = config.Exclusions.ExcludedMembers, | ||
| excluded_type_patterns = config.Exclusions.ExcludedTypePatterns, | ||
| excluded_member_patterns = config.Exclusions.ExcludedMemberPatterns | ||
| }, | ||
| breaking_change_rules = new | ||
| { | ||
| treat_type_removal_as_breaking = config.BreakingChangeRules.TreatTypeRemovalAsBreaking, | ||
| treat_member_removal_as_breaking = config.BreakingChangeRules.TreatMemberRemovalAsBreaking, | ||
| treat_signature_change_as_breaking = config.BreakingChangeRules.TreatSignatureChangeAsBreaking, | ||
| treat_reduced_accessibility_as_breaking = config.BreakingChangeRules.TreatReducedAccessibilityAsBreaking | ||
| }, | ||
| output_format = config.OutputFormat, | ||
| fail_on_breaking_changes = config.FailOnBreakingChanges, | ||
| output_path = config.OutputPath | ||
| }; |
There was a problem hiding this comment.
The PrepareConfigurationData method is duplicated with PrepareConfigData at line 80. Both methods appear to do similar things but with different object structures. Consider consolidating these methods to avoid code duplication.
| private object PrepareConfigurationData(DotNetApiDiff.Models.Configuration.ComparisonConfiguration config) | |
| { | |
| return new | |
| { | |
| filters = new | |
| { | |
| include_internals = config.Filters.IncludeInternals, | |
| include_compiler_generated = config.Filters.IncludeCompilerGenerated, | |
| include_namespaces = config.Filters.IncludeNamespaces, | |
| exclude_namespaces = config.Filters.ExcludeNamespaces, | |
| include_types = config.Filters.IncludeTypes, | |
| exclude_types = config.Filters.ExcludeTypes | |
| }, | |
| mappings = new | |
| { | |
| auto_map_same_name_types = config.Mappings.AutoMapSameNameTypes, | |
| ignore_case = config.Mappings.IgnoreCase, | |
| type_mappings = config.Mappings.TypeMappings.Select(kvp => new { key = kvp.Key, value = kvp.Value }), | |
| namespace_mappings = config.Mappings.NamespaceMappings.Select(kvp => new { key = kvp.Key, value = kvp.Value }) | |
| }, | |
| exclusions = new | |
| { | |
| excluded_types = config.Exclusions.ExcludedTypes, | |
| excluded_members = config.Exclusions.ExcludedMembers, | |
| excluded_type_patterns = config.Exclusions.ExcludedTypePatterns, | |
| excluded_member_patterns = config.Exclusions.ExcludedMemberPatterns | |
| }, | |
| breaking_change_rules = new | |
| { | |
| treat_type_removal_as_breaking = config.BreakingChangeRules.TreatTypeRemovalAsBreaking, | |
| treat_member_removal_as_breaking = config.BreakingChangeRules.TreatMemberRemovalAsBreaking, | |
| treat_signature_change_as_breaking = config.BreakingChangeRules.TreatSignatureChangeAsBreaking, | |
| treat_reduced_accessibility_as_breaking = config.BreakingChangeRules.TreatReducedAccessibilityAsBreaking | |
| }, | |
| output_format = config.OutputFormat, | |
| fail_on_breaking_changes = config.FailOnBreakingChanges, | |
| output_path = config.OutputPath | |
| }; | |
| private object PrepareConfigData(object config) | |
| { | |
| if (config is DotNetApiDiff.Models.Configuration.ComparisonConfiguration comparisonConfig) | |
| { | |
| return new | |
| { | |
| filters = new | |
| { | |
| include_internals = comparisonConfig.Filters.IncludeInternals, | |
| include_compiler_generated = comparisonConfig.Filters.IncludeCompilerGenerated, | |
| include_namespaces = comparisonConfig.Filters.IncludeNamespaces, | |
| exclude_namespaces = comparisonConfig.Filters.ExcludeNamespaces, | |
| include_types = comparisonConfig.Filters.IncludeTypes, | |
| exclude_types = comparisonConfig.Filters.ExcludeTypes | |
| }, | |
| mappings = new | |
| { | |
| auto_map_same_name_types = comparisonConfig.Mappings.AutoMapSameNameTypes, | |
| ignore_case = comparisonConfig.Mappings.IgnoreCase, | |
| type_mappings = comparisonConfig.Mappings.TypeMappings.Select(kvp => new { key = kvp.Key, value = kvp.Value }), | |
| namespace_mappings = comparisonConfig.Mappings.NamespaceMappings.Select(kvp => new { key = kvp.Key, value = kvp.Value }) | |
| }, | |
| exclusions = new | |
| { | |
| excluded_types = comparisonConfig.Exclusions.ExcludedTypes, | |
| excluded_members = comparisonConfig.Exclusions.ExcludedMembers, | |
| excluded_type_patterns = comparisonConfig.Exclusions.ExcludedTypePatterns, | |
| excluded_member_patterns = comparisonConfig.Exclusions.ExcludedMemberPatterns | |
| }, | |
| breaking_change_rules = new | |
| { | |
| treat_type_removal_as_breaking = comparisonConfig.BreakingChangeRules.TreatTypeRemovalAsBreaking, | |
| treat_member_removal_as_breaking = comparisonConfig.BreakingChangeRules.TreatMemberRemovalAsBreaking, | |
| treat_signature_change_as_breaking = comparisonConfig.BreakingChangeRules.TreatSignatureChangeAsBreaking, | |
| treat_reduced_accessibility_as_breaking = comparisonConfig.BreakingChangeRules.TreatReducedAccessibilityAsBreaking | |
| }, | |
| output_format = comparisonConfig.OutputFormat, | |
| fail_on_breaking_changes = comparisonConfig.FailOnBreakingChanges, | |
| output_path = comparisonConfig.OutputPath | |
| }; | |
| } | |
| // Add handling for other configuration types here if needed | |
| throw new ArgumentException("Unsupported configuration type", nameof(config)); |
| catch | ||
| { |
There was a problem hiding this comment.
Empty catch blocks without specific exception types can hide important errors. Consider catching specific exceptions or at least logging the error information.
| catch | |
| { | |
| catch (Exception ex) | |
| { | |
| Console.Error.WriteLine($"Error loading CSS styles: {ex.Message}"); |
| catch | ||
| { |
There was a problem hiding this comment.
Empty catch blocks without specific exception types can hide important errors. Consider catching specific exceptions or at least logging the error information.
| catch | |
| { | |
| catch (Exception ex) | |
| { | |
| Console.Error.WriteLine($"Error loading JavaScript: {ex.Message}"); |
- Fix CustomTemplateLoader to avoid using internal Scriban API - Replace Console.WriteLine with proper exception handling - Remove duplicate PrepareConfigurationData method - Improve error handling in GetCssStyles() and GetJavaScriptCode() - Use Debug.WriteLine for non-critical warnings instead of empty catch blocks These changes improve code robustness, eliminate code duplication, and follow better error handling practices.
Code Review ResponseI've reviewed and addressed all the feedback from the PR review. Here's my evaluation and the changes made: ✅ Comment 1: CustomTemplateLoader internal API usageIssue: Accessing ✅ Comment 2: Console.WriteLine for error loggingIssue: Using ✅ Comment 3: Duplicate PrepareConfigurationData methodIssue: Two similar methods ( ✅ Comments 4 & 5: Empty catch blocksIssue: Empty catch blocks in SummaryAll review feedback has been addressed with appropriate solutions that improve:
The formatter functionality remains intact and all tests continue to pass. The changes make the code more robust and follow better engineering practices. |
Overview
This PR replaces the StringBuilder-based HTML generation with a Scriban template engine implementation, providing better maintainability and separation of concerns while preserving single-file deployment capability.
Key Changes
New Files Added
main-layout.scriban- Main HTML document structurebreaking-changes.scriban- Breaking changes sectionchange-group.scriban- Individual change group displayconfiguration.scriban- Configuration details sectionconfig-*.scriban- Configuration subsection partialsstyles.css- Modern responsive CSS stylingscripts.js- Interactive JavaScript functionalityModified Files
Benefits
✅ Improved Maintainability: Template files are now separate and easily editable
✅ Better Separation of Concerns: Presentation logic separated from business logic
✅ Enhanced Styling: Modern responsive CSS with professional appearance
✅ Interactive UI: Collapsible configuration section and better UX
✅ Template Reusability: Include directives allow template composition
✅ Single-File Deployment: All templates embedded as resources during build
Testing
Breaking Changes
None - this is an internal implementation change that maintains the same public API.
Screenshots/Output
The generated HTML reports now feature:
Ready for review! 🚀