Extend the runtime formatting pipeline - #93
Conversation
Add a specialized formatter hook to execution contexts, distinguish generic fallback formatters, and centralize formatter resolution for both explicit Display calls and language kernels. Document the extension contract and cover priority, fallback, and failure behavior with tests. Signed-off-by: eosfor <9363027+eosfor@users.noreply.github.com>
Preserve raw PSObject results until the kernel can offer each runtime value to specialized IDataFormatter extensions. Batch contiguous unhandled objects through the existing PowerShell ETS renderer so native table formatting and mixed-output ordering remain intact. Signed-off-by: eosfor <9363027+eosfor@users.noreply.github.com>
7bfad83 to
3bfe332
Compare
|
Hi @TorreyBetts ! Before I mark this PR as ready for review, could you please confirm whether this direction fits Verso's extension architecture? The PR introduces a shared runtime formatting hook so language kernels can offer returned values to specialized IDataFormatter extensions while preserving their existing native formatting as the fallback. PR #94 provides a concrete consumer by implementing DataFrame formatting on top of this pipeline. All CI checks are currently green. If the API shape and the split between the core pipeline and the follow-up extension look appropriate, I’ll mark this PR as ready for review. If you would prefer a different integration point or scope, I’m happy to adjust it while it is still a draft. |
Thanks for asking before marking it ready. One change first. The rest looks good for a minor release. Both new interface members have default implementations, Additional contribution note: Coming up I need to start localizing, which will touch a lot of different files. Initially, it'll be UI of the VS Code extension and standalone server, then will be the different libraries. |
Signed-off-by: eosfor <9363027+eosfor@users.noreply.github.com>
|
Thanks — I’ve updated TryFormatAsync to accept an ordered list of acceptable MIME types. MIME preference order takes precedence over formatter priority, and the resolver skips outputs whose MIME type is not acceptable before trying the next formatter or representation. I’ll mark the PR ready for review once the current CI run completes. |
TorreyBetts
left a comment
There was a problem hiding this comment.
Thanks for turning the MIME list around so quickly. Three things I ran into after building the branch and putting a real formatter through it. All three are in the resolver path and I've left them inline.
|
|
||
| if (canFormat) | ||
| { | ||
| var formattedOutput = await formatter.FormatAsync(value, context).ConfigureAwait(false); |
There was a problem hiding this comment.
CanFormat is guarded but FormatAsync isn't, and the comment just above says a broken formatter must not prevent the kernel's native fallback from running. That only holds for the probe. I registered a formatter that throws here and ran 'good line 1'; 'good line 2' through the PowerShell kernel: the exception passes through PowerShellKernel.ExecuteAsync, which has no catch, up to the top level catch in ExecutionPipeline, and the cell reports failed with both good lines gone. Before this PR a formatter only ran on an explicit .Display(), so one bad extension couldn't take out unrelated output.
Could you extend the same try/continue over FormatAsync so a throwing formatter is treated as not having claimed the value? FormatterResolverTests covers the CanFormat throw but not this one.
There was a problem hiding this comment.
Fixed in a578b2a. FormatterResolver now guards FormatAsync as well as CanFormat. A formatter that throws is rejected for the current resolution, and the resolver continues to the next candidate or the kernel’s native fallback. Requested cancellation is still propagated rather than treated as a formatter failure.
| includeFallback: false).ConfigureAwait(false); | ||
| } | ||
|
|
||
| foreach (var mimeType in acceptableMimeTypes) |
There was a problem hiding this comment.
This calls the resolver once per acceptable type, so FormatAsync runs once per entry rather than once per value. With ["image/svg+xml", "image/png", "text/plain"] and a formatter that produces text/plain I get three FormatAsync calls and two results discarded. Any formatter doing real work pays for it three times, which is the DataFrame render in #94, and one writing progressive output through context.WriteOutputAsync emits it three times.
Could you probe with CanFormat under each hinted context but only call FormatAsync on the winner? That keeps MIME order ahead of priority and still resolves the value once.
There was a problem hiding this comment.
ExecutionContext now passes the complete acceptable MIME list to the resolver once. The resolver probes CanFormat across the MIME-specific contexts in preference order, uses formatter priority within each MIME type, and invokes FormatAsync only for the selected representation. A formatter that fails or returns a mismatched MIME type is not retried for the same value.
| ArgumentNullException.ThrowIfNull(value); | ||
| ArgumentNullException.ThrowIfNull(context); | ||
|
|
||
| if (value is CellOutput output) |
There was a problem hiding this comment.
This sits above the requiredMimeType check, so it returns whatever MIME the value already carries. TryFormatAsync(someCellOutput, ["image/png"]) gives back text/html on my build. The point of the acceptable list is the caller promising it can render what comes back, and this quietly breaks that.
It's also unreachable from display. DisplayHandler.DisplayAsync writes a CellOutput directly before it ever reaches the resolver, so the new path is the only live one. Could you drop it, or move it below the MIME check?
There was a problem hiding this comment.
I removed the early CellOutput return from FormatterResolver, so TryFormatAsync can no longer return a representation outside the caller’s acceptable MIME list. Explicit display behavior is unchanged because DisplayHandler already writes CellOutput values directly before reaching the resolver.
Signed-off-by: eosfor <9363027+eosfor@users.noreply.github.com>
|
Addressed all three resolver-path comments in a578b2a. The full local solution test suite passes, and all updated GitHub checks are green across Windows, macOS, Ubuntu, Python hosts, VS Code, DCO, and NuGet validation. Ready for another look. |
|
Everything looks good, I'll merge this. One note for a potential follow up if you'd like to pick it up. |
Summary
IExecutionContext.TryFormatAsyncFormatterResolver, including priority ordering, MIME hints, cancellation, and explicit fallback controlIDataFormatterextensions before using the kernel's native table/text renderingWhy
Language kernels can return rich runtime objects, but extension formatters previously participated consistently only in explicit display calls. PowerShell therefore formatted results inside the kernel before a specialized extension could handle them.
This change gives kernels a shared formatting entry point while preserving their existing native output as the fallback. It enables extensions for domain-specific runtime types without adding those dependencies to the core or to individual kernels.
Developer impact
IDataFormatter.IsFallbacklets generic formatters opt out of implicit kernel result handling while remaining available to explicitDisplaycalls. Existing formatter implementations remain source-compatible because the property has a default implementation.Validation
dotnet test Verso.sln --no-restoreThe full solution test suite passes. Existing dependency-analysis and documentation warnings are unchanged.
Follow-up
#94 uses this pipeline to render
Microsoft.Data.Analysis.DataFramevalues returned by PowerShell, providing a concrete end-to-end example of the extension point.