Switch spectre logger to MEL.Spectre#2619
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Good cleanup — dropping the pre-release dev channel dependency and unifying log-level filtering to a single mechanism () are both solid improvements. The interface implementation is correct. A few concerns worth discussing before merging:
1. Template is missing {Exception} — stack traces may disappear from console output
File: src/ModularPipelines/DependencyInjection/DependencyInjectionSetup.cs
The old per-level templates all ended with {NewLine}{Exception}:
profile.OutputTemplate = "[red][[Error]][/] {Message}{NewLine}{Exception}";
The new template is:
config.Template = "[{Level:u4}] {Message}";No {Exception} token. If MEL.Spectre does not append exceptions by default, then logger.LogError(ex, "message") calls will silently swallow the exception detail from the console — users will see the message but no stack trace. This is a significant behavioural regression for a pipeline framework where exception visibility is critical for debugging failures.
Suggestion: Verify whether MEL.Spectre appends exceptions automatically; if not, update the template:
config.Template = "[{Level:u4}] {Message}{NewLine}{Exception}";2. Exception stack-frame depth regressed from unlimited to default
File: src/ModularPipelines/DependencyInjection/DependencyInjectionSetup.cs
The old config explicitly rendered unlimited stack frames:
options.MaxStackFrames = int.MaxValue;The replacement:
config.ExceptionFormats = ExceptionFormats.Default;ExceptionFormats.Default from Spectre.Console is a basic rendering preset that doesn't guarantee unlimited frames. For a pipeline framework running multi-step build/deploy jobs, truncated stack traces make diagnosis harder, especially for deeply nested async call chains.
Suggestion: Check what MEL.Spectre exposes for controlling frame depth and either configure it explicitly or document that this is an intentional trade-off.
3. Per-level colour coding is gone
File: src/ModularPipelines/DependencyInjection/DependencyInjectionSetup.cs
The old setup rendered each level with a distinct Spectre markup colour (grey/blue/yellow/red/white-on-red). The new template "[{Level:u4}] {Message}" outputs everything with the same colour. This isn't a correctness issue but is a real UX regression — colour differentiation is the fastest way for a human watching a pipeline run to spot warnings or errors among a wall of info logs.
Suggestion: Check if MEL.Spectre supports a per-level colour style map. If so, configure it. If not, it's worth noting in the PR as a known trade-off.
4. PreserveMarkupInFormatStrings removed — markup in log messages may now be literal text
File: src/ModularPipelines/DependencyInjection/DependencyInjectionSetup.cs
The old config had:
profile.PreserveMarkupInFormatStrings = true;This was deliberate — it allowed callers to embed Spectre markup like [bold]text[/bold] directly in log messages and have it render. Removing it (or not having an equivalent in MEL.Spectre) means any existing code that relies on this will now display raw markup strings instead of styled output.
Suggestion: Audit whether any module code passes Spectre markup through ILogger calls, and verify MEL.Spectre's behaviour in that case.
Summary
| Change | Assessment |
|---|---|
| Drop pre-release dependency | ✅ Good |
Unify to single LoggerFilterOptions |
✅ Good |
Implement WriteAnsi interface member |
✅ Correct |
Template missing {Exception} |
|
ExceptionFormats.Default vs unlimited frames |
|
| Loss of per-level colours | |
PreserveMarkupInFormatStrings dropped |
The dependency swap is the right call; the main risk is in the MEL.Spectre configuration being under-specified compared to the old setup. Items 1 and 2 in particular could make pipeline failure diagnosis noticeably harder.
There was a problem hiding this comment.
Code Review
Good cleanup — dropping the pre-release vertical-spectreconsolelogger dev channel dependency and unifying log-level filtering to a single mechanism (LoggerFilterOptions) are both solid improvements. The WriteAnsi interface implementation is correct. A few concerns worth discussing before merging:
1. Template is missing {Exception} — stack traces may disappear from console output
File: src/ModularPipelines/DependencyInjection/DependencyInjectionSetup.cs
The old per-level templates all ended with {NewLine}{Exception}. The new template is:
config.Template = "[{Level:u4}] {Message}";No {Exception} token. If MEL.Spectre does not append exceptions by default, then logger.LogError(ex, "message") calls will silently swallow the exception detail from the console — users will see the message but no stack trace. This is a significant behavioural regression for a pipeline framework where exception visibility is critical for debugging failures.
Suggestion: Verify whether MEL.Spectre appends exceptions automatically; if not:
config.Template = "[{Level:u4}] {Message}{NewLine}{Exception}";2. Exception stack-frame depth regressed from unlimited to default
The old config explicitly rendered unlimited stack frames:
options.MaxStackFrames = int.MaxValue;The replacement config.ExceptionFormats = ExceptionFormats.Default is a basic rendering preset that does not guarantee unlimited frames. For a pipeline framework running multi-step build/deploy jobs, truncated stack traces make failure diagnosis harder, especially across deeply nested async call chains.
Suggestion: Check what MEL.Spectre exposes for controlling frame depth and configure it explicitly, or document that this is an intentional trade-off.
3. Per-level colour coding is gone
The old setup rendered each level with a distinct Spectre markup colour (grey/blue/yellow/red/white-on-red). The new template "[{Level:u4}] {Message}" outputs everything with the same colour. Colour differentiation is the fastest way for a human watching a pipeline run to spot warnings or errors among a wall of info logs.
Suggestion: Check if MEL.Spectre supports a per-level colour style map. If so, configure it. If not, worth noting in the PR as a known trade-off.
4. PreserveMarkupInFormatStrings removed — markup in log messages may now render as literal text
The old config had profile.PreserveMarkupInFormatStrings = true intentionally — it allowed callers to embed Spectre markup directly in log messages and have it render. Removing it (or not having an equivalent in MEL.Spectre) means any existing code that relies on this will display raw markup strings instead of styled output.
Suggestion: Audit whether any module code passes Spectre markup through ILogger calls, and verify MEL.Spectre's behaviour.
Summary
| Change | Assessment |
|---|---|
| Drop pre-release dependency | Good |
| Unify to single LoggerFilterOptions | Good |
| Implement WriteAnsi interface member | Correct |
| Template missing {Exception} | Likely regression |
| ExceptionFormats.Default vs unlimited frames | Worth verifying |
| Loss of per-level colours | UX regression |
| PreserveMarkupInFormatStrings dropped | Verify no callers rely on it |
The dependency swap is the right call; the main risk is in the MEL.Spectre configuration being under-specified compared to the old setup. Items 1 and 2 in particular could make pipeline failure diagnosis noticeably harder.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | ✅ 16 (≤ 20 complexity) |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Code Review (follow-up on second commit)
This is a follow-up review covering the second commit ("Fix CI console output polish") and tracking which prior concerns were addressed.
Status of previous review items
| Previous concern | Status |
|---|---|
Template missing {Exception} |
❌ Still not addressed — template remains "[{Level:u4}] {Message}" |
ExceptionFormats.Default vs unlimited stack frames |
❌ Still not addressed |
| Per-level colour coding gone | ❌ Still not addressed |
PreserveMarkupInFormatStrings removed |
✅ Addressed via AllowMarkupInMessageTemplate = true |
Items 1 and 2 remain the most impactful: exceptions logged via logger.LogError(ex, "msg") will lose their stack traces from console output unless MEL.Spectre appends them by default (which is not verified in this PR). For a pipeline framework where failure diagnosis is the main UX need, this is worth explicitly confirming.
New issues in the second commit
1. TrxFile path hardcodes Release configuration
File: src/ModularPipelines.Build/Modules/RunUnitTestsModule.cs
The DotNetRunOptions above uses Configuration = "Release", so this is consistent today, but the string is duplicated between two places. If the configuration ever changes (e.g. to Debug for local runs), the TRX lookup silently produces a path that doesn't exist, the if (!unitTestProject.TrxFile.Exists) guard swallows it, and skipped tests are silently omitted from the table with no indication.
Suggestion: Promote Configuration to a variable and reference it in both the run options and the TrxFile path:
2. RemoveSkippedTestDump strips all blank lines, not just the skipped block
File: src/ModularPipelines.Build/Modules/RunUnitTestsModule.cs
StringSplitOptions.RemoveEmptyEntries discards every empty/blank line in the entire output — not just within the skipped-test block being filtered. This will collapse whitespace in any section of the output (e.g. summary banners, spacing between test results).
Suggestion: Use StringSplitOptions.None and check for blank lines explicitly inside the block-skip logic:
3. Repeated hard cast to IConsoleWriter is fragile
Files: PrintEnvironmentVariablesModule.cs, RunUnitTestsModule.cs
This cast is repeated in two places and will throw InvalidCastException at runtime if the logger implementation ever changes (e.g. in a test host that provides a plain ILogger). It also leaks a build-module–specific concern (IConsoleWriter) into module code that should only know about IModuleContext.
Suggestion: If IModuleContext doesn't already expose a IConsoleWriter, consider exposing it directly (e.g. context.Console) so modules don't need to cast the logger. If the cast must remain for now, at least add a Debug.Assert or use as/null-check so the failure mode is explicit.
4. Unicode emoji in status label may not render on all CI terminals
File: src/ModularPipelines/Helpers/SpectreResultsPrinter.cs
The ⏭ character (U+23ED) is outside the Basic Multilingual Plane subset that many CI terminal fonts cover. Windows console environments and some Linux CI runners without full emoji font support will render it as ? or a box. The other status strings use text only or ASCII. This is minor but inconsistent.
New code that is well done
- The negative-guard on
savedTimeinSpectreResultsPrinter.csis a correct bug fix — the previous code could produce a negative "Saved" value when wall clock exceeded total module time. - Removing the redundant empty row before the totals row in the summary table is clean.
- The
SkippedTest/UnitTestProjectsealed records are a nice, lightweight way to group the related data. Markup.Escapeis correctly applied to all user-controlled values in the new tables — no markup injection risk.- Removing the
Expand = truefrom the modules table will prevent it from stretching to fill wide terminals where content is short.
There was a problem hiding this comment.
Code Review (follow-up on second commit)
This is a follow-up review covering the second commit ("Fix CI console output polish") and tracking which prior concerns were addressed.
Status of previous review items
| Previous concern | Status |
|---|---|
Template missing {Exception} |
Not addressed — template remains "[{Level:u4}] {Message}" |
ExceptionFormats.Default vs unlimited stack frames |
Not addressed |
| Per-level colour coding gone | Not addressed |
PreserveMarkupInFormatStrings removed |
Addressed via AllowMarkupInMessageTemplate = true |
Items 1 and 2 remain the most impactful: exceptions logged via logger.LogError(ex, "msg") will lose their stack traces from console output unless MEL.Spectre appends them by default (which is not verified in this PR). For a pipeline framework where failure diagnosis is the main UX need, this is worth explicitly confirming.
New issues in the second commit
1. TrxFile path hardcodes Release configuration
File: src/ModularPipelines.Build/Modules/RunUnitTestsModule.cs
The TrxFile property resolves to bin/Release/{Framework}/TestResults/ but the Release string is not tied to the Configuration = "Release" in DotNetRunOptions — they are separate literals. If the configuration ever changes, the TRX lookup silently produces a path that doesn't exist. The if (!unitTestProject.TrxFile.Exists) guard will swallow the miss and skipped tests disappear from the table with no warning.
Suggestion: Extract the configuration string to a variable, pass it into UnitTestProject, and reference it in both DotNetRunOptions and TrxFile. This way the two values cannot diverge.
2. RemoveSkippedTestDump strips all blank lines, not just the skipped block
File: src/ModularPipelines.Build/Modules/RunUnitTestsModule.cs
output.Split(['\r', '\n'], StringSplitOptions.RemoveEmptyEntries)
RemoveEmptyEntries discards every blank line in the entire output, not just within the skipped-test block being filtered. Any spacing between test result sections (summary banners, separator lines, etc.) will be collapsed.
Suggestion: Use StringSplitOptions.None and only skip blank lines when skippingSkippedBlock is true; pass all other blank lines through as-is.
3. Repeated hard cast to IConsoleWriter is fragile
Files: PrintEnvironmentVariablesModule.cs, RunUnitTestsModule.cs
((IConsoleWriter)context.Logger).Write(table);This cast will throw InvalidCastException at runtime if the logger implementation changes (e.g. in a test host providing a plain ILogger). It also leaks a build-specific concern into module code that should only know about IModuleContext.
Suggestion: If IModuleContext does not already expose a console-writer surface, consider exposing it directly (e.g. context.Console.Write(table)) so modules don't need to downcast the logger. At minimum, guard with as/null-check so the failure is explicit rather than an unhandled cast exception mid-pipeline.
4. Unicode emoji in status label may not render on all CI terminals
File: src/ModularPipelines/Helpers/SpectreResultsPrinter.cs
ModuleStatus.Skipped => "[dim]⏭ skipped[/]",The ⏭ character is outside the subset that many CI terminal fonts cover. All other status strings use text only. This is a minor inconsistency but could produce a ? or box on Windows console or constrained Linux runners.
New code that looks good
- The negative guard on
savedTimeinSpectreResultsPrinter.csis a correct bug fix — the previous code could display a negative "Saved" value. - Removing the empty row before the totals row is a clean simplification.
- The
SkippedTest/UnitTestProjectsealed records are a tidy way to group related data without overhead. Markup.Escapeis correctly applied to all user-controlled values in the new tables — no markup injection risk.- Removing
Expand = truefrom the modules table prevents it from stretching to fill wide terminals unnecessarily.
1 participant
Summary
vertical-spectreconsoleloggerwithMEL.Spectre0.4.0 and enable markup support in MEL.Spectre templatesIAnsiConsole.WriteAnsimember required by Spectre.Console 0.55.2Fixes #2620
Fixes #2621
Fixes #2622
Fixes #2623
Verification
dotnet restore D:\git\ModularPipelines\ModularPipelines.slndotnet build D:\git\ModularPipelines\ModularPipelines.sln --no-restoreNote: a targeted TUnit run using
--treenode-filtertimed out in this environment and left childdotnetprocesses, which were stopped. The solution build passes against the restoredMEL.Spectre0.4.0 package.