From 50a604f169ff4e346ed98b22cbbe5f5e9a81bc0d Mon Sep 17 00:00:00 2001 From: bircni Date: Mon, 12 Oct 2026 10:00:05 +0200 Subject: [PATCH] refactor: remove helpers the new output and prompts left unused Drop writeDownload, terminal.Require, terminal.Confirm, terminal.Row, terminal.Record and Pulls.FindOpenByHead, which no command calls anymore, and point their tests at the code that replaced them. Signed-off-by: bircni --- internal/app/download.go | 14 ----------- internal/app/download_test.go | 11 +++++---- internal/services/pulls.go | 26 -------------------- internal/services/pulls_test.go | 9 ++++--- internal/terminal/streams.go | 11 --------- internal/terminal/streams_test.go | 6 ++--- internal/terminal/table.go | 35 --------------------------- internal/terminal/table_test.go | 40 +++++++++++++++---------------- 8 files changed, 34 insertions(+), 118 deletions(-) diff --git a/internal/app/download.go b/internal/app/download.go index dbecb1ea..7e560c78 100644 --- a/internal/app/download.go +++ b/internal/app/download.go @@ -19,7 +19,6 @@ import ( "gitea.dev/tea/internal/clierr" "gitea.dev/tea/internal/terminal" "github.com/spf13/cobra" - "golang.org/x/term" ) // downloadFlags are the destination flags of a download command. @@ -95,19 +94,6 @@ func (a *application) download(cmd *cobra.Command, f downloadFlags, path string, }) } -func writeDownload(cmd *cobra.Command, body io.Reader, output string) error { - if output == "" || output == "-" { - return writeTerminalSafe(cmd.OutOrStdout(), isTerminal(cmd.OutOrStdout()), body) - } - return publish(cmd, output, false, false, func(destination io.Writer) error { _, err := io.Copy(destination, body); return err }) -} - -// isTerminal reports whether w is a terminal, for writers without injected stream capabilities. -func isTerminal(w io.Writer) bool { - file, ok := w.(*os.File) - return ok && term.IsTerminal(int(file.Fd())) -} - // binarySniffLength is how much of a download decides whether it is text. const binarySniffLength = 8 << 10 diff --git a/internal/app/download_test.go b/internal/app/download_test.go index e7549a87..2168a27b 100644 --- a/internal/app/download_test.go +++ b/internal/app/download_test.go @@ -40,21 +40,24 @@ func TestDownloadsPublishOnlyCompleteFiles(t *testing.T) { cmd.SetContext(context.Background()) var stdout bytes.Buffer cmd.SetOut(&stdout) - require.NoError(t, writeDownload(cmd, bytes.NewBufferString("archive"), "-")) + copyBody := func(body io.Reader) func(io.Writer) error { + return func(destination io.Writer) error { _, err := io.Copy(destination, body); return err } + } + require.NoError(t, writeTerminalSafe(cmd.OutOrStdout(), false, bytes.NewBufferString("archive"))) require.Equal(t, "archive", stdout.String()) directory := t.TempDir() output := filepath.Join(directory, "logs.zip") - require.ErrorIs(t, writeDownload(cmd, failingDownload{}, output), io.ErrUnexpectedEOF) + require.ErrorIs(t, publish(cmd, output, false, false, copyBody(failingDownload{})), io.ErrUnexpectedEOF) _, err := os.Stat(output) require.True(t, errors.Is(err, os.ErrNotExist)) entries, err := os.ReadDir(directory) require.NoError(t, err) require.Empty(t, entries) - require.NoError(t, writeDownload(cmd, bytes.NewBufferString("archive"), output)) + require.NoError(t, publish(cmd, output, false, false, copyBody(bytes.NewBufferString("archive")))) info, err := os.Stat(output) require.NoError(t, err) require.Equal(t, umaskMode(t), info.Mode().Perm(), "downloads are not private files") - require.ErrorContains(t, writeDownload(cmd, bytes.NewBufferString("replacement"), output), "already exists") + require.ErrorContains(t, publish(cmd, output, false, false, copyBody(bytes.NewBufferString("replacement"))), "already exists") data, err := os.ReadFile(output) require.NoError(t, err) require.Equal(t, "archive", string(data)) diff --git a/internal/services/pulls.go b/internal/services/pulls.go index 2a32361f..bbfac283 100644 --- a/internal/services/pulls.go +++ b/internal/services/pulls.go @@ -250,32 +250,6 @@ func (s Pulls) ListOpen(ctx context.Context) ([]Pull, error) { }) } -// FindOpenByHead returns the unique open pull whose head branch matches name. -func (s Pulls) FindOpenByHead(ctx context.Context, head string) (Pull, error) { - head = strings.TrimSpace(head) - if head == "" { - return Pull{}, fmt.Errorf("head branch cannot be empty") - } - items, err := s.ListOpen(ctx) - if err != nil { - return Pull{}, err - } - matches := []Pull{} - for _, item := range items { - if item.Head == head { - matches = append(matches, item) - } - } - switch len(matches) { - case 0: - return Pull{}, fmt.Errorf("no open pull request uses head branch %q", head) - case 1: - return matches[0], nil - default: - return Pull{}, fmt.Errorf("multiple open pull requests use head branch %q; supply an explicit number", head) - } -} - // Create opens a pull request. func (s Pulls) Create(ctx context.Context, opts PullOptions) (Pull, error) { // TODO(sdk): CreatePullRequestOption lacks allow_maintainer_edit. diff --git a/internal/services/pulls_test.go b/internal/services/pulls_test.go index e228b9ba..4cd5742d 100644 --- a/internal/services/pulls_test.go +++ b/internal/services/pulls_test.go @@ -57,7 +57,7 @@ func TestPullMilestoneClearUsesIssueEndpoint(t *testing.T) { } } -func TestFindOpenByHeadPagesPastSmallServerLimitsAndMatchesExactly(t *testing.T) { +func TestListOpenPagesPastSmallServerLimits(t *testing.T) { client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { assert.Equal(t, "/api/v1/repos/alice/project/pulls", r.URL.Path) switch r.URL.Query().Get("page") { @@ -70,11 +70,10 @@ func TestFindOpenByHeadPagesPastSmallServerLimitsAndMatchesExactly(t *testing.T) } })) service := Pulls{Client: client, Repository: "alice/project"} - item, err := service.FindOpenByHead(context.Background(), "feature") + items, err := service.ListOpen(context.Background()) require.NoError(t, err) - require.EqualValues(t, 2, item.Number) - _, err = service.FindOpenByHead(context.Background(), "FEATURE") - require.ErrorContains(t, err, "no open pull request") + require.Len(t, items, 2) + require.Equal(t, "feature", items[1].Head) } func TestListOpenRejectsEndlessPagination(t *testing.T) { diff --git a/internal/terminal/streams.go b/internal/terminal/streams.go index 107b40ba..767f3c14 100644 --- a/internal/terminal/streams.go +++ b/internal/terminal/streams.go @@ -205,11 +205,6 @@ func RequireInput(ctx context.Context, value *string, flag, label string, valida return true, nil } -// Require is RequireInput titled by the flag name, for callers without a human label. -func Require(ctx context.Context, value *string, flag string, streams Streams, policy Policy, prompt Prompter) (bool, error) { - return RequireInput(ctx, value, flag, flag, nil, streams, policy, prompt) -} - // Confirmation describes the question asked before an action. type Confirmation struct { // Prompt asks about the action in sentence case: `Delete label "bug" from alice/x?`. @@ -267,9 +262,3 @@ func ConfirmAction(ctx context.Context, c Confirmation, yes bool, streams Stream } return err } - -// Confirm requires explicit acceptance unless confirmation was supplied by flag. -// It is ConfirmAction for callers that predate Confirmation. -func Confirm(ctx context.Context, description string, yes bool, streams Streams, policy Policy, prompt Prompter) error { - return ConfirmAction(ctx, Confirmation{Prompt: description}, yes, streams, policy, prompt) -} diff --git a/internal/terminal/streams_test.go b/internal/terminal/streams_test.go index 502dfceb..a80212b3 100644 --- a/internal/terminal/streams_test.go +++ b/internal/terminal/streams_test.go @@ -56,12 +56,12 @@ func TestPromptPolicy(t *testing.T) { } prompt := &fakePrompt{} value := "supplied" - asked, err := Require(context.Background(), &value, "title", Streams{}, Policy{}, prompt) + asked, err := RequireInput(context.Background(), &value, "title", "title", nil, Streams{}, Policy{}, prompt) require.NoError(t, err) require.False(t, asked) require.Zero(t, prompt.inputs) value = "" - _, err = Require(context.Background(), &value, "title", Streams{}, Policy{}, prompt) + _, err = RequireInput(context.Background(), &value, "title", "title", nil, Streams{}, Policy{}, prompt) require.ErrorContains(t, err, "--title") } @@ -141,7 +141,7 @@ func TestConfirmActionPassesTheDefault(t *testing.T) { prompt := &askingPrompt{accept: true} streams := Streams{InTTY: true, ErrTTY: true} require.NoError(t, ConfirmAction(context.Background(), Confirmation{Prompt: "Create pull request?", DefaultYes: true}, false, streams, Policy{Enabled: true}, prompt)) - require.NoError(t, Confirm(context.Background(), "Delete branch?", false, streams, Policy{Enabled: true}, prompt)) + require.NoError(t, ConfirmAction(context.Background(), Confirmation{Prompt: "Delete branch?"}, false, streams, Policy{Enabled: true}, prompt)) require.Equal(t, []bool{true, false}, prompt.defaults) require.Equal(t, []string{"Create pull request?", "Delete branch?"}, prompt.titles) } diff --git a/internal/terminal/table.go b/internal/terminal/table.go index c1584957..c3cab655 100644 --- a/internal/terminal/table.go +++ b/internal/terminal/table.go @@ -84,15 +84,6 @@ func (c Cell) style(text string, d Display, link bool) string { return Hyperlink(c.Link, text, link && d.Links) } -// Row returns unstyled cells. -func Row(texts ...string) []Cell { - cells := make([]Cell, len(texts)) - for i, text := range texts { - cells[i].Text = text - } - return cells -} - // Table renders rows under columns; rows may have more cells than there are columns. On a TTY it // prints an uppercase header row when any column has a header, aligns columns two spaces apart, // and shrinks Flex columns until rows fit the display width. Otherwise it writes one line of @@ -209,32 +200,6 @@ func fitWidth(widths []int, limit int, flex func(j int) (bool, int)) { } } -// Record is one item for human readers: a heading line of cells separated by spaces, -// followed by text lines such as a body and a URL. -type Record struct { - Heading []Cell - Lines []string -} - -// Render formats the record. Heading cells pass through SafeLine and are styled like -// Table cells; lines pass through SafeText, so they keep their own line breaks. -func (r Record) Render(color bool) string { - d := Display{TTY: true, Color: color, TrueColor: true, Links: color} - var b strings.Builder - for i, cell := range r.Heading { - if i > 0 { - b.WriteByte(' ') - } - b.WriteString(cell.style(SafeLine(cell.Text), d, true)) - } - b.WriteByte('\n') - for _, line := range r.Lines { - b.WriteString(SafeText(line)) - b.WriteByte('\n') - } - return b.String() -} - // SafeLine reduces server text to one line: SafeText, with tabs and line breaks as spaces. func SafeLine(s string) string { return strings.Map(func(r rune) rune { diff --git a/internal/terminal/table_test.go b/internal/terminal/table_test.go index ee113460..7b57099e 100644 --- a/internal/terminal/table_test.go +++ b/internal/terminal/table_test.go @@ -28,11 +28,11 @@ func TestTable(t *testing.T) { want string }{ {"empty", issues, nil, tty, ""}, - {"aligns legacy rows without header", nil, [][]Cell{Row("7", "Fix login", "open"), Row("12", "Crash", "closed")}, tty, "7 Fix login open\n12 Crash closed\n"}, - {"counts runes, not bytes", nil, [][]Cell{Row("1", "äöü", "x"), Row("2", "abcd", "y")}, tty, "1 äöü x\n2 abcd y\n"}, - {"counts wide runes twice", nil, [][]Cell{Row("1", "修复", "x"), Row("2", "abcd", "y")}, tty, "1 修复 x\n2 abcd y\n"}, - {"trims empty trailing cells", nil, [][]Cell{Row("1", "bug", ""), Row("22", "feature", "new work")}, tty, "1 bug\n22 feature new work\n"}, - {"pads ragged rows", nil, [][]Cell{Row("1"), Row("2", "two")}, tty, "1\n2 two\n"}, + {"aligns legacy rows without header", nil, [][]Cell{row("7", "Fix login", "open"), row("12", "Crash", "closed")}, tty, "7 Fix login open\n12 Crash closed\n"}, + {"counts runes, not bytes", nil, [][]Cell{row("1", "äöü", "x"), row("2", "abcd", "y")}, tty, "1 äöü x\n2 abcd y\n"}, + {"counts wide runes twice", nil, [][]Cell{row("1", "修复", "x"), row("2", "abcd", "y")}, tty, "1 修复 x\n2 abcd y\n"}, + {"trims empty trailing cells", nil, [][]Cell{row("1", "bug", ""), row("22", "feature", "new work")}, tty, "1 bug\n22 feature new work\n"}, + {"pads ragged rows", nil, [][]Cell{row("1"), row("2", "two")}, tty, "1\n2 two\n"}, { "heads columns and formats cells for people", issues, issueRows, tty, @@ -59,7 +59,7 @@ func TestTable(t *testing.T) { { "cuts wide runes by display width", []Column{{Header: "Title", Flex: true}, {Header: "By"}}, - [][]Cell{Row("支持中文路径 (support CJK paths)", "alice")}, + [][]Cell{row("支持中文路径 (support CJK paths)", "alice")}, Display{TTY: true, Width: 20}, "TITLE BY\n支持中文路径… alice\n", }, @@ -73,21 +73,21 @@ func TestTable(t *testing.T) { { "keeps columns to their output", []Column{{Header: "ID"}, {Header: "State", PipedOnly: true}, {Header: "Link", TTYOnly: true}}, - [][]Cell{Row("1", "open", "→")}, + [][]Cell{row("1", "open", "→")}, tty, "ID LINK\n1 →\n", }, { "keeps piped-only columns when piped", []Column{{Header: "ID"}, {Header: "State", PipedOnly: true}, {Header: "Link", TTYOnly: true}}, - [][]Cell{Row("1", "open", "→")}, + [][]Cell{row("1", "open", "→")}, Display{}, "1\topen\n", }, { "strips hostile text in both outputs", nil, - [][]Cell{Row("1", "a\x1b]52;c;cHduZWQ=\x07b\x1b[2J\u009b31m‮txt.exe\r\ntail\tend", "open")}, + [][]Cell{row("1", "a\x1b]52;c;cHduZWQ=\x07b\x1b[2J\u009b31m‮txt.exe\r\ntail\tend", "open")}, Display{}, "1\ta]52;c;cHduZWQ=b[2J31mtxt.exe tail end\topen\n", }, @@ -95,7 +95,7 @@ func TestTable(t *testing.T) { { "styles after alignment and links only the first column", []Column{{Header: "ID"}, {Header: "Name"}, {Header: "State"}}, - [][]Cell{{{Text: "7", Link: "https://example.org/7"}, {Text: "bug", Color: "ff0000", Link: "https://example.org/bug"}, {Text: "open", Color: "green", Bold: true}}, Row("12", "feature", "closed")}, + [][]Cell{{{Text: "7", Link: "https://example.org/7"}, {Text: "bug", Color: "ff0000", Link: "https://example.org/bug"}, {Text: "open", Color: "green", Bold: true}}, row("12", "feature", "closed")}, Display{TTY: true, Color: true, Links: true}, "\x1b[90mID\x1b[0m \x1b[90mNAME\x1b[0m \x1b[90mSTATE\x1b[0m\n" + "\x1b]8;;https://example.org/7\x1b\\7\x1b]8;;\x1b\\ \x1b[38;5;196mbug\x1b[0m \x1b[1m\x1b[32mopen\x1b[0m\x1b[0m\n" + @@ -126,7 +126,7 @@ func TestTableFitsWidthWithWideRunes(t *testing.T) { titles := []string{"Emoji in title 🚀 works?", "支持中文路径 (support CJK paths)", strings.Repeat("long ", 40)} rows := make([][]Cell, len(titles)) for i, title := range titles { - rows[i] = Row("#1", title, "alice") + rows[i] = row("#1", title, "alice") } for _, width := range []int{30, 50, 80} { for _, line := range strings.Split(strings.TrimSuffix(Table([]Column{{}, {Flex: true}, {}}, rows, Display{TTY: true, Width: width}), "\n"), "\n") { @@ -135,15 +135,6 @@ func TestTableFitsWidthWithWideRunes(t *testing.T) { } } -func TestRecord(t *testing.T) { - record := Record{ - Heading: []Cell{{Text: "#7", Bold: true}, {Text: "Crash\x1b[2J\nnow"}, {Text: "bob‮"}}, - Lines: []string{"Steps:\r\n\x1b]52;c;aGk=\x07run\n", "https://example.org/7"}, - } - require.Equal(t, "#7 Crash[2J now bob\nSteps:\n]52;c;aGk=run\n\nhttps://example.org/7\n", record.Render(false)) - require.Equal(t, "\x1b[1m#7\x1b[0m Crash[2J now bob\nSteps:\n]52;c;aGk=run\n\nhttps://example.org/7\n", record.Render(true)) -} - func TestSafeText(t *testing.T) { for _, tc := range []struct{ name, in, text, line string }{ {"plain", "fix login", "fix login", "fix login"}, @@ -162,3 +153,12 @@ func TestSafeText(t *testing.T) { }) } } + +// row returns unstyled cells. +func row(texts ...string) []Cell { + cells := make([]Cell, len(texts)) + for i, text := range texts { + cells[i].Text = text + } + return cells +}