fix: --include-feedback has no effect on default (table) output format - #273
Open
SpiliosDmk (SpiliosDimakopoulos) wants to merge 1 commit into
Conversation
output.PrintRunsTable never accepted an includeFeedback parameter, so none of its three call sites (run list, trace list, thread get) could pass through the --include-feedback / --full flag that already exists on those commands. The feedback_stats field was already being fetched and placed into each run's map by extract.ExtractRun -- it just never reached the table renderer, so `--include-feedback` silently had no effect on the default "pretty" (table) format, even though it worked correctly for JSON output. Fixes langchain-ai#186 Changes: - output.PrintRunsTable(w, runs, includeMetadata, includeFeedback, title) now takes includeFeedback and, when true, adds a "Feedback" column. - Added formatFeedbackStats(), a small helper that renders each run's feedback_stats map as a compact single-line summary: - numeric feedback: "key=avg (n=N)" - categorical feedback: "key{label=count,...}" - multiple keys are joined with "; ", sorted for determinism - keys with zero recorded points are skipped - Updated the three call sites (run.go, trace.go, thread.go) to pass the includeFeedback variable that was already in scope at each site. Tests: - Updated the existing TestPrintRunsTable call to the new 5-arg signature, and added an assertion that no "Feedback" column appears when includeFeedback is false. - Added TestPrintRunsTable_WithFeedback, exercising both a run with numeric + categorical feedback and a run with none. - Added TestFormatFeedbackStats with table-driven cases: empty map, numeric, categorical, multi-key sort order, and a key with n=0 being skipped. Verification note: this repo's go.mod requires go >= 1.25.0, which isn't available in my sandbox (max installable via apt is go1.23, and proxy.golang.org / sum.golang.org are not reachable). I verified this change by copying internal/output/{output.go,output_test.go} verbatim into an isolated throwaway module (pinning the exact same tablewriter/treeprint versions declared in this repo's go.mod, with local `replace` directives redirecting a couple of gopkg.in-hosted transitive test-only deps to their GitHub mirrors purely to work around my sandbox's network allowlist) and ran `go build` + `go test -v` there with go1.23: all 9 test functions passed, including the 3 new/updated ones. I also independently compiled and ran just the new formatFeedbackStats logic against 6 hand-written cases with go1.23, and confirmed with `gofmt -l` that every touched file is correctly formatted. None of these workarounds are part of this patch -- please still run `go build ./...` and `go test ./...` in a proper go1.25 environment before merging.
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
1 participant
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.
Fixes #186
Problem
output.PrintRunsTablenever accepted anincludeFeedbackparameter,so none of its three call sites (
run list,trace list,thread get)could pass through the
--include-feedback/--fullflag thatalready exists on those commands.
feedback_statswas already beingfetched and placed into each run's map by
extract.ExtractRun— itjust never reached the table renderer.
Fix
output.PrintRunsTable(w, runs, includeMetadata, includeFeedback, title)now adds a "Feedback" column when
includeFeedbackis true.formatFeedbackStats(): renders numeric feedback askey=avg (n=N)and categorical feedback askey{label=count,...},joining multiple keys with
;in sorted order.includeFeedbackvariable.Tests
TestPrintRunsTablefor the new signature; added anassertion that no Feedback column appears when the flag is off.
TestPrintRunsTable_WithFeedbackand table-drivenTestFormatFeedbackStats(empty, numeric, categorical, multi-keysort order, zero-
nkey skipped).Verification
My sandbox can't install Go 1.25 (this repo's go.mod requirement),
so I verified by copying the real
internal/output/{output.go, output_test.go}into an isolated module pinning this repo's exacttablewriter/treeprint versions, and ran
go build+go test -vwith go1.23 — all 9 tests passed. Also independently compiled/ran the
new
formatFeedbackStatslogic against 6 cases, and confirmedgofmt -lis clean on every touched file. Please still rungo build ./.../go test ./...in CI/locally before merging.