Merge v13 into develop - #331
Conversation
* Replace MiKTeX/LaTeX reporting with Markdown + QuestPDF Removes the dependency on OSPSuite.TeXReporting and MiKTeX, replacing it with a simpler, pure .NET solution: - Markdown reports with embedded SVG charts (viewable in VS Code, GitHub) - PDF reports via QuestPDF (no external tools required) - Custom SVG chart generator for 2D line charts with log/linear scale - Report format switchable via ReportFormat enum New files: - Reporting/Charts/SvgChartGenerator.cs - SVG line chart generation - Reporting/Markdown/* - Markdown builder pattern implementation - Reporting/Pdf/PdfReportDocument.cs - QuestPDF document - Services/MarkdownReportingTask.cs, PdfReportingTask.cs Removed: - All *TeXBuilder.cs files - *Reporter.cs files - OSPSuite.TeXReporting dependency from all projects Closes #183 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Restore fixProductNameForWindows11 method for tests The method was removed during cross-platform refactoring but is needed by OperatingSystemInfoSpecs tests that use reflection to call it. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Revert OperatingSystemInfo to original Windows-specific implementation Keep the original Windows-specific OperatingSystemInfo that uses Registry, WMI, and SystemInformation APIs. Update Core and Tests projects to target net8.0-windows. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Trigger update * Address PR comments * Refactor test to use real MarkdownBuilderRepository Replace TestMarkdownBuilderRepository with the real MarkdownBuilderRepository and all production builders. This improves test coverage by exercising the actual builder implementations rather than duplicating their logic. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Add MissingFileComparisonResult handling in Markdown and PDF reports - Create MissingFileComparisonResultMarkdownBuilder to properly report missing file details including which folder contains the file and which folder is missing it - Add ComposeMissingFileResult in PdfReportDocument to display the missing file validation message in PDF reports - Register the new builder in ValidatorRegister - Add builder to test setup Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Fix PR review findings: table escaping, null checks, and test refactoring - Escape pipe characters in MarkdownReportContext.AppendTable to prevent broken markdown tables when cell values contain '|' - Add null validation for installationValidationResult and RunSummary in both MarkdownReportingTask and PdfReportingTask before accessing StartTime - Refactor MarkdownReportingSpecs to use ContextForIntegration, resolving IMarkdownReportingTask from the DI container instead of manually constructing all builders Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Refactor ValidationReportingTask to use ReportOptions parameter Replace DefaultFormat property with ReportOptions parameter that includes: - ReportFormat as flags enum (None, Markdown, Pdf, All) - OpenReport boolean - Convenience properties ExportToMarkdown and ExportToPdf This allows generating both Markdown and PDF reports in a single call by using ReportFormat.All. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Bump to version 13. ALso use correct builder * Fix Markdown bold formatting by removing trailing space from ValidationResult caption The trailing space in the caption prevented proper bold formatting in Markdown output (**Result of the validation: ** instead of **Result of the validation:**). * Only generate charts for invalid outputs in Markdown and PDF reports Valid outputs with matching curves don't need charts. Removing the valid-outputs-with-data loop prevents generating thousands of unnecessary pages when a file has many outputs but only a few are invalid. * Ensures that we export the expected format * Add all valid curves back * Use report-specific filenames for PDF output Pass reportName parameter to reportOutputPath so PDF filenames reflect the actual report type (FolderComparison vs InstallationValidation). * Extract shared helpers, disable start without report format, add tests - Extract ChartDataFactory.CreateFor() to eliminate duplicate createChartData methods in OutputComparisonResultMarkdownBuilder and PdfReportDocument - Extract ColorExtensions.ToHexString() to replace colorToHex in SvgChartGenerator, toQuestColor in PdfReportDocument, and inline hex formatting in MarkdownReportContext - Fix PDF run summary labels: start/end time now have their own labels instead of reusing BatchRunDuration for the start time - Disable start button in MainView and SimulationComparisonView when neither PDF nor Markdown report format is selected - Replace Enumerable.Select allocation with Enumerable.Repeat in MarkdownReportContext.AppendTable separator row - Add MarkdownReportContextSpecs unit tests covering headings, tables, colored status, bold formatting, and content accumulation - Add observations to MarkdownReportingSpecs for SVG chart, validation state, and deviation section * Replace hardcoded strings with resources and add null guards - Add localized resource keys to Assets.Reporting: StartTime, EndTime, Yes, No, OSLabel, FileWasContainedInFolder, ButWasMissingInFolder, ValidationPerformedIn - Replace all hardcoded English strings in PdfReportDocument with Assets.Reporting resources for consistent localization - Add constructor null guards in PdfReportDocument for validationResult, comparisonResult, and svgChartGenerator - Add null guard for comparisonResult in both PdfReportingTask and MarkdownReportingTask CreateReport overloads * Ensure enable state is reset --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: msevestre <msevestre@users.noreply.github.com>
|
Important Review skippedToo many files! This PR contains 52 files, which is 2 over the limit of 50. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (66)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Bump nightly version 12.3->13.0, target net10.0-windows output paths, and update actions/checkout@v6 and actions/upload-artifact@v7. Set build-and-test temp-version and coverage build version to 13.0. Point nightly-badge at the renamed build-nightly_13.0 workflow.
Overview and review by Perplexity :)Here's a full code review of PR #331 — "Merge v13 into develop" in the InstallationValidator repository. TL;DRThe PR's headline change is a complete replacement of MiKTeX/LaTeX reporting with a Markdown + QuestPDF pipeline, alongside a .NET 10 upgrade and a bump of OSPSuite.Core to 13.0.134. The architecture is clean and the removal of the TeX external dependency is a real improvement — but there are a few issues worth addressing before merging. [github](https://github.com/Open-Systems-Pharmacology/InstallationValidator/pulls) Findings at a Glance🔴 Must-Fix (3 items)
🟡 Should-Fix (5 items)
🟢 Informational
Architecture AssessmentThe new reporting layer is well-designed. It uses the same The report covers all the details, findings, and the merge recommendation. |
The issues should be addressed later, not in this PR |
3 participants
Promotes the v13 (13.0) line to
develop, replacing the 12.x line.developwas on the 12.x maintenance line (Core 12.3.32) whilev13is the 13.0 line (Core 13.0.119). The two develop-only commits were 12.3-specific (Core version bump 12.3.1 -> 12.3.32, batch-output regeneration, dimensions bump). To makedevelopbecome exactly the 13.0 tree,origin/developwas merged intov13with-s ours(recording develop as a merged parent while keeping v13's tree), then two wanted items were ported forward on top:c144fe1(2026-05-19, strictly ahead of v13's71bf282from 2025-08-20).data/Outputs/BatchFiles/*.json) taken from develop (newer than v13's).The 12.3 Core version bump was intentionally dropped — v13 stays on OSPSuite.Core 13.0.119.
developis now a strict ancestor ofv13, so this PR applies cleanly.Workflow branch triggers already targeted
developon v13, so no retarget was needed.Note: InstallationValidator v13 references OSPSuite.Core 13.0.119 (not yet bumped to the latest 13.0.134), and its nightly workflow is still named
build-nightly_12.3.yml(cosmetic).Please merge with a merge commit (do not squash or rebase).
🤖 Generated with Claude Code