From a0334820d2ae851f6464ad3e5d3e3439cc29a0b5 Mon Sep 17 00:00:00 2001 From: patramsey Date: Wed, 19 Aug 2026 18:32:13 -0600 Subject: [PATCH 1/2] fix: correct exit codes, unknown subcommands, and table overflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An audit of the CLI surface turned up nine defects, all of them in how the binary presents itself rather than in what it asks the API for. Three of them break scripted use: A mistyped subcommand succeeded. Cobra's legacyArgs only rejects unknown commands on the root command — for a parent that itself has a parent it returns nil, so cobra fell through to "not runnable", printed help, and returned no error. `namecom domain regsiter example.com` exited 0, which means `namecom domain regsiter foo.com && deploy` deployed. Every group is now wrapped in cmdutil.GroupCmd, which rejects an unknown subcommand as a usage error and reuses cobra's suggestion list. A group invoked bare still prints help and exits 0, which is what browsing looks like. Invocation mistakes exited 1 instead of the documented 2. Two causes: the validators in cmd/cmdutil/validate.go returned bare fmt.Errorf values, and cobra checks required flags inside execute(), after SetFlagErrorFunc has had its chance and with no hook of its own. So `--type ZZZ` and a missing `--answer` reported the same code as a 500 while `--badflag` beside them reported 2. Validators now build UsageError directly; cobra's own messages are classified in one documented place, asserted by a test that fails if an upgrade rewords them. --dry-run promised to "print the API request that would be sent without executing it", but only the 28 write paths consult it — reads always called the API. Rather than change what 38 commands do, the flag now says what it does. The rest are presentation. Tables were rendered at natural width with no regard for the terminal: 113 columns for `domain list` against an 80-column pane, where the rounded borders wrap into fragments. Trailing columns are dropped until the table fits, a footer names what went, and --wide opts out; a pipe has no width to fit and keeps everything. Relative dates widen their unit past a quarter, because "in 2750 days" told a reader nothing. Examples moved above the flag tables. Command groups no longer advertise themselves as `namecom domain [flags]`. `dns create --type` lists CAA, which the validator has always accepted. Non-string flag defaults print unquoted. CLAUDE.md's command tree had drifted — no config, contact, status, open, or version, and `vanity/` where the command is `vanity-ns`. --- CHANGELOG.md | 37 +++++++++ cmd/auth.go | 1 + cmd/cmdutil/args.go | 45 +++++++++++ cmd/cmdutil/args_test.go | 53 +++++++++++++ cmd/cmdutil/errors.go | 41 ++++++++++ cmd/cmdutil/errors_test.go | 55 +++++++++++++ cmd/cmdutil/validate.go | 91 +++++++++++---------- cmd/config/config.go | 1 + cmd/contact/contact.go | 1 + cmd/dns/dns.go | 3 +- cmd/dnssec/dnssec.go | 1 + cmd/domain/domain.go | 1 + cmd/domain/manage.go | 1 + cmd/email/email.go | 1 + cmd/groups_test.go | 55 +++++++++++++ cmd/help.go | 50 ++++++++---- cmd/order/order.go | 1 + cmd/root.go | 9 ++- cmd/transfer/transfer.go | 1 + cmd/url/url.go | 1 + cmd/vanity/vanity.go | 1 + internal/output/output.go | 141 +++++++++++++++++++++++++++++---- internal/output/output_test.go | 113 ++++++++++++++++++++++++++ 23 files changed, 628 insertions(+), 76 deletions(-) create mode 100644 cmd/groups_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e6d48b5..2cd023a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,43 @@ Releases before `0.2.0` predate this file. Their notes are on the ## [Unreleased] +### Added +- `--wide` keeps every table column even when the table is wider than the + terminal. + +### Fixed +- A mistyped subcommand now fails instead of succeeding. Cobra checks for + unknown commands only on the root command, so `namecom domain regsiter + example.com` printed the group's help and exited **0** — meaning + `namecom domain regsiter foo.com && deploy` ran `deploy`. Every command + group now rejects an unknown subcommand as a usage error (exit 2) and + offers the same "Did you mean this?" suggestion the root command does. + Invoking a group bare still prints its help and exits 0. +- Invocation mistakes now exit **2**, as the documented exit-code table has + always claimed. Flag-value validation returned unclassified errors and + cobra's own required-flag check runs where `SetFlagErrorFunc` cannot see + it, so `--type ZZZ` and a missing `--answer` both exited 1 — the same code + a script uses to detect a server error — while `--badflag` beside them + exited 2. +- Tables no longer overflow the terminal. They rendered at natural width + regardless of it (`domain list` came to 113 columns, `order list` 99, + `dns list` 87), so in an 80-column pane the rounded borders wrapped into + fragments. Trailing columns are now dropped until the table fits, with a + footer naming what was hidden; `--wide` restores them, and piped output is + unaffected. +- Relative dates widen their unit past a quarter, so a domain paid through + 2034 reads `in 8 years` rather than `in 2750 days`. +- `--dry-run` said it printed "the API request that would be sent without + executing it", but only write operations honour it; reads always called the + API. The flag now says so rather than implying an invocation touches + nothing. +- `dns create --type` omitted `CAA` from its list of record types, which the + validator has always accepted. +- Help pages put `Examples:` directly under the usage line instead of below + the flag tables and footer, command groups show `namecom ` + instead of the uninvokable `namecom [flags]`, and non-string flag + defaults print unquoted (`default 300`, not `default "300"`). + ## [0.2.4] - 2026-08-17 ### Changed diff --git a/cmd/auth.go b/cmd/auth.go index eb9ee05..ad9e921 100644 --- a/cmd/auth.go +++ b/cmd/auth.go @@ -48,6 +48,7 @@ var logoutProfile string func init() { authLoginCmd.Flags().StringVar(&loginProfile, "profile", "default", "profile name to save credentials under") authLogoutCmd.Flags().StringVar(&logoutProfile, "profile", "", "profile to remove (defaults to the active profile)") + cmdutil.GroupCmd(authCmd) authCmd.AddCommand(authLoginCmd, authStatusCmd, authLogoutCmd) rootCmd.AddCommand(authCmd) } diff --git a/cmd/cmdutil/args.go b/cmd/cmdutil/args.go index cd94295..7f31c1e 100644 --- a/cmd/cmdutil/args.go +++ b/cmd/cmdutil/args.go @@ -81,3 +81,48 @@ func joinNames(names []string) string { return strings.Join(names[:len(names)-1], ", ") + ", and " + names[len(names)-1] } } + +// GroupCmd wires a command group — a parent that exists only to hold +// subcommands — so a mistyped subcommand fails instead of succeeding quietly. +// +// Cobra only checks for unknown commands on the ROOT command: legacyArgs() +// returns nil for any parent that itself has a parent. So `namecom domian` +// errored with a suggestion, while `namecom domain regsiter example.com` +// printed the group's help and exited 0. In a script that reads as success — +// `namecom domain regsiter foo.com && deploy` ran deploy. +// +// Bare `namecom domain` keeps its old behavior of printing help and exiting 0, +// which is what a user typing a group name to browse it expects. +func GroupCmd(cmd *cobra.Command) *cobra.Command { + // Cobra defaults this to 2, but only on the root command, inside the + // unknown-command path we are replacing here. Left at zero, SuggestionsFor + // matches nothing and every typo loses its "Did you mean" line. + if cmd.SuggestionsMinimumDistance <= 0 { + cmd.SuggestionsMinimumDistance = 2 + } + cmd.Args = func(c *cobra.Command, args []string) error { + if len(args) == 0 { + return nil + } + return NewUsageError(fmt.Errorf("unknown command %q for %q%s", + args[0], c.CommandPath(), suggestionHint(c.SuggestionsFor(args[0])))) + } + cmd.RunE = func(c *cobra.Command, _ []string) error { + return c.Help() + } + return cmd +} + +// suggestionHint renders cobra's near-miss list the way cobra renders it on the +// root command, so a typo reads the same wherever in the tree it happens. +func suggestionHint(suggestions []string) string { + if len(suggestions) == 0 { + return "" + } + var b strings.Builder + b.WriteString("\n\nDid you mean this?\n") + for _, s := range suggestions { + fmt.Fprintf(&b, "\t%s\n", s) + } + return b.String() +} diff --git a/cmd/cmdutil/args_test.go b/cmd/cmdutil/args_test.go index a88448f..8c689b0 100644 --- a/cmd/cmdutil/args_test.go +++ b/cmd/cmdutil/args_test.go @@ -1,6 +1,7 @@ package cmdutil import ( + "errors" "strings" "testing" @@ -161,3 +162,55 @@ func TestExactArgs_HintContainsUseLine(t *testing.T) { t.Errorf("error = %q, missing command path in hint", err.Error()) } } + +// TestGroupCmd guards the unknown-subcommand check on command groups. +// +// Cobra's legacyArgs only rejects unknown commands on the ROOT command — for +// any parent that itself has a parent it returns nil, so cobra fell through to +// "not runnable", printed help, and returned no error. `namecom domain +// regsiter example.com` therefore exited 0, and +// `namecom domain regsiter foo.com && deploy` deployed. +func TestGroupCmd(t *testing.T) { + newGroup := func() *cobra.Command { + root := &cobra.Command{Use: "namecom"} + group := GroupCmd(&cobra.Command{Use: "domain"}) + group.AddCommand(&cobra.Command{Use: "register", Run: func(*cobra.Command, []string) {}}) + group.AddCommand(&cobra.Command{Use: "list", Run: func(*cobra.Command, []string) {}}) + root.AddCommand(group) + return group + } + + t.Run("unknown subcommand is a usage error", func(t *testing.T) { + group := newGroup() + err := group.Args(group, []string{"regsiter", "example.com"}) + if err == nil { + t.Fatal("unknown subcommand accepted; it would print help and exit 0") + } + var u *UsageError + if !errors.As(err, &u) { + t.Errorf("error is not a UsageError, so it would exit 1 not 2: %v", err) + } + if !strings.Contains(err.Error(), `unknown command "regsiter"`) { + t.Errorf("error does not name the typo: %v", err) + } + if !strings.Contains(err.Error(), "register") { + t.Errorf("error carries no suggestion: %v", err) + } + }) + + t.Run("no args is allowed so the group can print help", func(t *testing.T) { + group := newGroup() + if err := group.Args(group, nil); err != nil { + t.Errorf("bare group name rejected: %v", err) + } + }) + + t.Run("suggestion distance is set", func(t *testing.T) { + // Cobra defaults this to 2 only inside the root-command path being + // replaced here. Left at zero, SuggestionsFor matches nothing. + group := newGroup() + if group.SuggestionsMinimumDistance <= 0 { + t.Error("SuggestionsMinimumDistance not set; every typo loses its hint") + } + }) +} diff --git a/cmd/cmdutil/errors.go b/cmd/cmdutil/errors.go index 75616b2..f8dd2ad 100644 --- a/cmd/cmdutil/errors.go +++ b/cmd/cmdutil/errors.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "net/http" + "strings" "github.com/patramsey/namecom-cli/internal/api" ) @@ -88,3 +89,43 @@ func AsRestricted(err error, operation, program string) error { } return err } + +// cobraUsagePrefixes are the messages cobra produces for invocation mistakes it +// validates itself, after our own hooks have run. +// +// SetFlagErrorFunc in root.go covers flag *parsing*, but cobra checks required +// flags and flag groups later, inside execute(), and offers no hook for that +// path — a missing required flag surfaced as a bare error and exited 1 while +// `--badflag` right next to it exited 2. Matching the message is unpleasant but +// it is the only seam cobra exposes; the strings are stable and asserted by +// TestClassifyCobraUsage, which fails loudly if an upgrade rewords one. +var cobraUsagePrefixes = []string{ + "required flag(s) ", + "if any flags in the group ", + "unknown command ", + "unknown flag: ", + "unknown shorthand flag: ", + "invalid argument ", + "flag needs an argument", +} + +// ClassifyCobraUsage wraps cobra's own invocation errors as UsageError so they +// reach the documented exit code 2. Errors that are already classified, and +// errors from anywhere else, pass through untouched. +func ClassifyCobraUsage(err error) error { + if err == nil { + return nil + } + var usage *UsageError + var auth *AuthError + if errors.As(err, &usage) || errors.As(err, &auth) { + return err + } + msg := err.Error() + for _, p := range cobraUsagePrefixes { + if strings.Contains(msg, p) { + return NewUsageError(err) + } + } + return err +} diff --git a/cmd/cmdutil/errors_test.go b/cmd/cmdutil/errors_test.go index c7a27ba..b1baba1 100644 --- a/cmd/cmdutil/errors_test.go +++ b/cmd/cmdutil/errors_test.go @@ -84,3 +84,58 @@ func TestNewUsageError(t *testing.T) { t.Error("NewUsageError should preserve the original error in the chain") } } + +// TestClassifyCobraUsage guards the message matching that maps cobra's own +// invocation errors onto exit code 2. +// +// Cobra validates required flags and flag groups inside execute(), after +// SetFlagErrorFunc has had its chance, and exposes no hook for that path. So +// `dns create example.com` with two required flags missing exited 1 while +// `dns list example.com --badflag` right beside it exited 2. Matching the +// message is the only seam available; these cases fail if a cobra upgrade +// rewords one, which is the point of asserting them. +func TestClassifyCobraUsage(t *testing.T) { + usage := func(err error) bool { + var u *UsageError + return errors.As(err, &u) + } + + cobraMessages := []string{ + `required flag(s) "answer", "type" not set`, + `if any flags in the group [a b] are set they must all be set; missing [b]`, + `unknown command "regsiter" for "namecom domain"`, + `unknown flag: --badflag`, + `unknown shorthand flag: 'z' in -z`, + `invalid argument "abc" for "--ttl" flag: strconv.ParseInt: parsing "abc": invalid syntax`, + `flag needs an argument: --type`, + } + for _, msg := range cobraMessages { + if got := ClassifyCobraUsage(errors.New(msg)); !usage(got) { + t.Errorf("ClassifyCobraUsage(%q) did not classify as a usage error", msg) + } + } + + t.Run("leaves other errors alone", func(t *testing.T) { + runtime := errors.New("connection refused") + if got := ClassifyCobraUsage(runtime); usage(got) { + t.Errorf("misclassified a runtime error as usage: %v", got) + } + }) + + t.Run("nil stays nil", func(t *testing.T) { + if got := ClassifyCobraUsage(nil); got != nil { + t.Errorf("ClassifyCobraUsage(nil) = %v, want nil", got) + } + }) + + t.Run("does not re-wrap an already-classified error", func(t *testing.T) { + // An AuthError whose text happens to mention an unknown flag must keep + // its exit code 3 rather than being demoted to a usage error. + auth := NewAuthError(errors.New("unknown flag: --token was not usable")) + got := ClassifyCobraUsage(auth) + var a *AuthError + if !errors.As(got, &a) { + t.Errorf("ClassifyCobraUsage demoted an AuthError: %v", got) + } + }) +} diff --git a/cmd/cmdutil/validate.go b/cmd/cmdutil/validate.go index 5d1c6c9..eed6f68 100644 --- a/cmd/cmdutil/validate.go +++ b/cmd/cmdutil/validate.go @@ -8,10 +8,19 @@ import ( "time" ) +// usagef builds a UsageError. Every failure in this file is an invocation +// mistake — a malformed flag value, an out-of-range count, an argument that +// cannot be what it claims — so each one maps to exit code 2 in the documented +// table. They used to return bare fmt.Errorf values, which collapsed to exit 1 +// and made `--type ZZZ` indistinguishable from a 500 to a calling script. +func usagef(format string, a ...any) error { + return NewUsageError(fmt.Errorf(format, a...)) +} + // ValidDate checks that s is a valid YYYY-MM-DD date. func ValidDate(s, flagName string) error { if _, err := time.Parse("2006-01-02", s); err != nil { - return fmt.Errorf("--%s: %q is not a valid date — expected YYYY-MM-DD", flagName, s) + return usagef("--%s: %q is not a valid date — expected YYYY-MM-DD", flagName, s) } return nil } @@ -24,7 +33,7 @@ var validDNSTypes = map[string]bool{ // ValidDNSType checks that t is a supported DNS record type. func ValidDNSType(t string) error { if !validDNSTypes[strings.ToUpper(t)] { - return fmt.Errorf("unknown record type %q — must be one of: A, AAAA, ANAME, CAA, CNAME, MX, NS, SRV, TXT", t) + return usagef("unknown record type %q — must be one of: A, AAAA, ANAME, CAA, CNAME, MX, NS, SRV, TXT", t) } return nil } @@ -32,27 +41,27 @@ func ValidDNSType(t string) error { // ValidDNSHost checks that host is a valid relative DNS label (@ and wildcards allowed). func ValidDNSHost(host string) error { if host == "" { - return fmt.Errorf("--host cannot be empty (use @ for the zone apex)") + return usagef("--host cannot be empty (use @ for the zone apex)") } if host == "@" || host == "*" { return nil } check := strings.TrimPrefix(host, "*.") if strings.ContainsAny(check, " \t") { - return fmt.Errorf("--host %q must not contain spaces", host) + return usagef("--host %q must not contain spaces", host) } if len(check) > 253 { - return fmt.Errorf("--host %q exceeds maximum DNS name length (253 chars)", host) + return usagef("--host %q exceeds maximum DNS name length (253 chars)", host) } for label := range strings.SplitSeq(check, ".") { if label == "" { - return fmt.Errorf("--host %q has an empty label (double dot or leading/trailing dot)", host) + return usagef("--host %q has an empty label (double dot or leading/trailing dot)", host) } if len(label) > 63 { - return fmt.Errorf("--host %q label %q exceeds 63 characters", host, label) + return usagef("--host %q label %q exceeds 63 characters", host, label) } if strings.HasPrefix(label, "-") || strings.HasSuffix(label, "-") { - return fmt.Errorf("--host %q label %q must not start or end with a hyphen", host, label) + return usagef("--host %q label %q must not start or end with a hyphen", host, label) } } return nil @@ -61,50 +70,50 @@ func ValidDNSHost(host string) error { // ValidDNSAnswer validates the answer field for a given record type and host. func ValidDNSAnswer(recordType, host, answer string) error { if answer == "" { - return fmt.Errorf("--answer is required") + return usagef("--answer is required") } switch strings.ToUpper(recordType) { case "A": ip := net.ParseIP(answer) if ip == nil || ip.To4() == nil { - return fmt.Errorf("--answer must be a valid IPv4 address for A records, got %q", answer) + return usagef("--answer must be a valid IPv4 address for A records, got %q", answer) } case "AAAA": ip := net.ParseIP(answer) if ip == nil || ip.To4() != nil { - return fmt.Errorf("--answer must be a valid IPv6 address for AAAA records, got %q", answer) + return usagef("--answer must be a valid IPv6 address for AAAA records, got %q", answer) } case "CNAME": if host == "@" || host == "" { - return fmt.Errorf("CNAME record cannot be set at the zone apex (@) — use ANAME for apex aliasing") + return usagef("CNAME record cannot be set at the zone apex (@) — use ANAME for apex aliasing") } case "MX": if strings.ContainsAny(answer, " \t") { - return fmt.Errorf("MX record --answer must be a hostname (got %q) — set priority with --priority", answer) + return usagef("MX record --answer must be a hostname (got %q) — set priority with --priority", answer) } case "SRV": parts := strings.Fields(answer) if len(parts) != 3 { - return fmt.Errorf("SRV record --answer must be \"weight port target\" (e.g. \"0 443 target.example.com.\"), got %q", answer) + return usagef("SRV record --answer must be \"weight port target\" (e.g. \"0 443 target.example.com.\"), got %q", answer) } if _, err := strconv.Atoi(parts[0]); err != nil { - return fmt.Errorf("SRV record weight (first field) must be an integer, got %q", parts[0]) + return usagef("SRV record weight (first field) must be an integer, got %q", parts[0]) } if _, err := strconv.Atoi(parts[1]); err != nil { - return fmt.Errorf("SRV record port (second field) must be an integer, got %q", parts[1]) + return usagef("SRV record port (second field) must be an integer, got %q", parts[1]) } case "CAA": parts := strings.Fields(answer) if len(parts) < 3 { - return fmt.Errorf("CAA record --answer must be \"flags tag value\" (e.g. `0 issue \"letsencrypt.org\"`), got %q", answer) + return usagef("CAA record --answer must be \"flags tag value\" (e.g. `0 issue \"letsencrypt.org\"`), got %q", answer) } flags, err := strconv.Atoi(parts[0]) if err != nil || flags < 0 || flags > 255 { - return fmt.Errorf("CAA record flags (first field) must be an integer 0-255, got %q", parts[0]) + return usagef("CAA record flags (first field) must be an integer 0-255, got %q", parts[0]) } validTags := map[string]bool{"issue": true, "issuewild": true, "iodef": true} if !validTags[parts[1]] { - return fmt.Errorf("CAA record tag (second field) must be one of: issue, issuewild, iodef — got %q", parts[1]) + return usagef("CAA record tag (second field) must be one of: issue, issuewild, iodef — got %q", parts[1]) } } return nil @@ -152,7 +161,7 @@ func isPrivateIP(ip net.IP) bool { // ValidTTL checks that ttl meets the API minimum. func ValidTTL(ttl int64) error { if ttl < 300 { - return fmt.Errorf("--ttl must be at least 300 seconds (got %d)", ttl) + return usagef("--ttl must be at least 300 seconds (got %d)", ttl) } return nil } @@ -160,13 +169,13 @@ func ValidTTL(ttl int64) error { // ValidDomainName does a basic sanity check on a domain name argument. func ValidDomainName(domain string) error { if strings.Contains(domain, " ") { - return fmt.Errorf("domain name %q must not contain spaces", domain) + return usagef("domain name %q must not contain spaces", domain) } if !strings.Contains(domain, ".") { - return fmt.Errorf("domain name %q must contain at least one dot", domain) + return usagef("domain name %q must contain at least one dot", domain) } if strings.HasPrefix(domain, ".") || strings.HasSuffix(domain, ".") { - return fmt.Errorf("domain name %q must not start or end with a dot", domain) + return usagef("domain name %q must not start or end with a dot", domain) } return nil } @@ -174,20 +183,20 @@ func ValidDomainName(domain string) error { // ValidNameserver checks that ns is a plausible fully-qualified nameserver hostname. func ValidNameserver(ns string, idx int) error { if ns == "" { - return fmt.Errorf("nameserver %d is empty", idx+1) + return usagef("nameserver %d is empty", idx+1) } if !strings.Contains(ns, ".") { - return fmt.Errorf("nameserver %q must be a fully-qualified hostname (e.g. ns1.example.com)", ns) + return usagef("nameserver %q must be a fully-qualified hostname (e.g. ns1.example.com)", ns) } if strings.HasPrefix(ns, ".") || strings.HasSuffix(ns, ".") { - return fmt.Errorf("nameserver %q must not start or end with a dot", ns) + return usagef("nameserver %q must not start or end with a dot", ns) } for label := range strings.SplitSeq(ns, ".") { if label == "" { - return fmt.Errorf("nameserver %q has an empty label", ns) + return usagef("nameserver %q has an empty label", ns) } if strings.HasPrefix(label, "-") || strings.HasSuffix(label, "-") { - return fmt.Errorf("nameserver %q label %q must not start or end with a hyphen", ns, label) + return usagef("nameserver %q label %q must not start or end with a hyphen", ns, label) } } return nil @@ -205,13 +214,13 @@ func ValidSortDir(dir string) error { case "", "asc", "desc": return nil } - return NewUsageError(fmt.Errorf("invalid --sort-dir %q: valid values are asc, desc", dir)) + return usagef("invalid --sort-dir %q: valid values are asc, desc", dir) } // ValidYears checks that n is a valid domain registration/renewal period. func ValidYears(n int) error { if n < 1 || n > 10 { - return fmt.Errorf("--years must be between 1 and 10 (got %d)", n) + return usagef("--years must be between 1 and 10 (got %d)", n) } return nil } @@ -219,11 +228,11 @@ func ValidYears(n int) error { // ValidURL checks that u has an http:// or https:// scheme. func ValidURL(u, flagName string) error { if u == "" { - return fmt.Errorf("--%s is required", flagName) + return usagef("--%s is required", flagName) } lower := strings.ToLower(u) if !strings.HasPrefix(lower, "http://") && !strings.HasPrefix(lower, "https://") { - return fmt.Errorf("--%s %q must start with http:// or https://", flagName, u) + return usagef("--%s %q must start with http:// or https://", flagName, u) } return nil } @@ -231,14 +240,14 @@ func ValidURL(u, flagName string) error { // ValidEmail checks that addr looks like a valid email address. func ValidEmail(addr, flagName string) error { if addr == "" { - return fmt.Errorf("--%s is required", flagName) + return usagef("--%s is required", flagName) } at := strings.LastIndex(addr, "@") if at < 1 || at == len(addr)-1 { - return fmt.Errorf("--%s %q is not a valid email address — expected user@domain.tld", flagName, addr) + return usagef("--%s %q is not a valid email address — expected user@domain.tld", flagName, addr) } if !strings.Contains(addr[at+1:], ".") { - return fmt.Errorf("--%s %q domain part has no dot — expected user@domain.tld", flagName, addr) + return usagef("--%s %q domain part has no dot — expected user@domain.tld", flagName, addr) } return nil } @@ -249,19 +258,19 @@ func ValidURLForwardingType(t, flagName string) error { case "redirect", "302", "masked": return nil } - return fmt.Errorf("--%s %q is not valid — must be one of: redirect, 302, masked", flagName, t) + return usagef("--%s %q is not valid — must be one of: redirect, 302, masked", flagName, t) } // ValidEmailLocalPart checks that s is a valid email mailbox local-part. func ValidEmailLocalPart(s, argName string) error { if s == "" { - return fmt.Errorf("%s must not be empty", argName) + return usagef("%s must not be empty", argName) } if strings.Contains(s, "@") { - return fmt.Errorf("%s %q must not contain '@' — provide only the local part (e.g. 'info', not 'info@example.com')", argName, s) + return usagef("%s %q must not contain '@' — provide only the local part (e.g. 'info', not 'info@example.com')", argName, s) } if strings.ContainsAny(s, " \t\n") { - return fmt.Errorf("%s %q must not contain spaces", argName, s) + return usagef("%s %q must not contain spaces", argName, s) } return nil } @@ -293,10 +302,10 @@ func CanonicalDomain(s string) string { // ValidAuthCode checks that a transfer auth code is plausibly non-trivial. func ValidAuthCode(code string) error { if code == "" { - return fmt.Errorf("--auth-code is required") + return usagef("--auth-code is required") } if len(code) < 6 { - return fmt.Errorf("--auth-code is too short (got %d chars, minimum 6); EPP auth codes are typically 8+ characters", len(code)) + return usagef("--auth-code is too short (got %d chars, minimum 6); EPP auth codes are typically 8+ characters", len(code)) } return nil } diff --git a/cmd/config/config.go b/cmd/config/config.go index a249570..b689d20 100644 --- a/cmd/config/config.go +++ b/cmd/config/config.go @@ -46,6 +46,7 @@ var showCmd = &cobra.Command{ } func init() { + cmdutil.GroupCmd(Cmd) Cmd.AddCommand(listProfilesCmd, useCmd, showCmd) } diff --git a/cmd/contact/contact.go b/cmd/contact/contact.go index d8671e5..68178e9 100644 --- a/cmd/contact/contact.go +++ b/cmd/contact/contact.go @@ -81,6 +81,7 @@ contact click the link.`, func init() { unverifiedCmd.Flags().BoolVar(&listAll, "all", false, "fetch all pages") + cmdutil.GroupCmd(Cmd) Cmd.AddCommand(unverifiedCmd, resendCmd, verifyCmd) } diff --git a/cmd/dns/dns.go b/cmd/dns/dns.go index f1013e2..0554bba 100644 --- a/cmd/dns/dns.go +++ b/cmd/dns/dns.go @@ -131,7 +131,7 @@ func init() { listCmd.Flags().BoolVar(&listAll, "all", false, "fetch all pages automatically") listCmd.Flags().StringVar(&listType, "type", "", "filter by record type (A, AAAA, CNAME, MX, TXT, NS, SRV, ANAME, CAA)") - createCmd.Flags().StringVar(&createType, "type", "", "record type: A, AAAA, ANAME, CNAME, MX, NS, SRV, TXT (required)") + createCmd.Flags().StringVar(&createType, "type", "", "record type: A, AAAA, ANAME, CAA, CNAME, MX, NS, SRV, TXT (required)") createCmd.Flags().StringVar(&createHost, "host", "@", "hostname relative to the zone (@ for apex)") createCmd.Flags().StringVar(&createAnswer, "answer", "", "record value (required)") createCmd.Flags().Int64Var(&createTTL, "ttl", 300, "TTL in seconds (minimum 300)") @@ -151,6 +151,7 @@ func init() { importCmd.Flags().BoolVar(&importDryRun, "dry-run", false, "show what would be created without calling the API") _ = importCmd.MarkFlagRequired("file") + cmdutil.GroupCmd(Cmd) Cmd.AddCommand(listCmd, createCmd, updateCmd, deleteCmd, exportCmd, importCmd) } diff --git a/cmd/dnssec/dnssec.go b/cmd/dnssec/dnssec.go index e72987a..7be3efb 100644 --- a/cmd/dnssec/dnssec.go +++ b/cmd/dnssec/dnssec.go @@ -71,6 +71,7 @@ func init() { _ = createCmd.MarkFlagRequired("digest-type") _ = createCmd.MarkFlagRequired("key-tag") + cmdutil.GroupCmd(Cmd) Cmd.AddCommand(listCmd, getCmd, createCmd, deleteCmd) } diff --git a/cmd/domain/domain.go b/cmd/domain/domain.go index 20a38f8..1f8e593 100644 --- a/cmd/domain/domain.go +++ b/cmd/domain/domain.go @@ -14,6 +14,7 @@ var Cmd = &cobra.Command{ } func init() { + cmdutil.GroupCmd(Cmd) Cmd.AddCommand( listCmd, getCmd, diff --git a/cmd/domain/manage.go b/cmd/domain/manage.go index 0f78c76..e7b9749 100644 --- a/cmd/domain/manage.go +++ b/cmd/domain/manage.go @@ -285,6 +285,7 @@ var contactsFile string func init() { contactsSetCmd.Flags().StringVar(&contactsFile, "from-file", "", "JSON file with contact data (required)") _ = contactsSetCmd.MarkFlagRequired("from-file") + cmdutil.GroupCmd(contactsCmd) contactsCmd.AddCommand(contactsGetCmd, contactsSetCmd) } diff --git a/cmd/email/email.go b/cmd/email/email.go index bd1cc28..22c3173 100644 --- a/cmd/email/email.go +++ b/cmd/email/email.go @@ -80,6 +80,7 @@ func init() { createCmd.Flags().StringVar(&createEmailTo, "to", "", "destination email address (required)") updateCmd.Flags().StringVar(&updateEmailTo, "to", "", "new destination email address") + cmdutil.GroupCmd(Cmd) Cmd.AddCommand(listCmd, getCmd, createCmd, updateCmd, deleteCmd) } diff --git a/cmd/groups_test.go b/cmd/groups_test.go new file mode 100644 index 0000000..8c69ff9 --- /dev/null +++ b/cmd/groups_test.go @@ -0,0 +1,55 @@ +package cmd + +import ( + "errors" + "strings" + "testing" + + "github.com/patramsey/namecom-cli/cmd/cmdutil" + "github.com/spf13/cobra" +) + +// TestEveryGroupRejectsUnknownSubcommands walks the real command tree and +// asserts that every parent group is wired through cmdutil.GroupCmd. +// +// The unit test in cmd/cmdutil covers the helper; this one covers the wiring, +// which is the part that actually regresses. Cobra checks for unknown commands +// only on the root, so a group added later without GroupCmd silently goes back +// to printing help and exiting 0 for a typo — and nothing else would notice. +func TestEveryGroupRejectsUnknownSubcommands(t *testing.T) { + var walk func(*cobra.Command) + walk = func(c *cobra.Command) { + for _, sub := range c.Commands() { + walk(sub) + } + // Only groups: commands that hold subcommands and are not the root. + if !c.HasAvailableSubCommands() || !c.HasParent() { + return + } + // cobra's generated `completion` tree is not ours to police. + if c.Name() == "completion" { + return + } + + t.Run(c.CommandPath(), func(t *testing.T) { + if c.Args == nil { + t.Fatalf("%q has subcommands but no Args validator — a typo'd subcommand exits 0; wrap it in cmdutil.GroupCmd", c.CommandPath()) + } + err := c.Args(c, []string{"nosuchsubcommand"}) + if err == nil { + t.Fatalf("%q accepted an unknown subcommand", c.CommandPath()) + } + var u *cmdutil.UsageError + if !errors.As(err, &u) { + t.Errorf("%q rejects unknown subcommands but not as a UsageError, so it exits 1 instead of 2: %v", c.CommandPath(), err) + } + if !strings.Contains(err.Error(), "nosuchsubcommand") { + t.Errorf("%q error does not name the offending word: %v", c.CommandPath(), err) + } + if err := c.Args(c, nil); err != nil { + t.Errorf("%q rejects being invoked bare, which should print help: %v", c.CommandPath(), err) + } + }) + } + walk(rootCmd) +} diff --git a/cmd/help.go b/cmd/help.go index b49b87d..a01c62a 100644 --- a/cmd/help.go +++ b/cmd/help.go @@ -51,7 +51,19 @@ func printHelp(w io.Writer, cmd *cobra.Command, color bool) { // Usage line fmt.Fprintln(w, style(helpHeading, "Usage:")) - fmt.Fprintf(w, " %s\n\n", style(helpUsage, cmd.UseLine())) + fmt.Fprintf(w, " %s\n\n", style(helpUsage, usageLine(cmd))) + + // Aliases and examples sit directly under the usage line. Examples used to + // print last, below the flag tables and the "see all global options" + // footer, which put the most-read part of a help page furthest down it. + if len(cmd.Aliases) > 0 { + fmt.Fprintf(w, "%s %s\n\n", style(helpHeading, "Aliases:"), strings.Join(cmd.Aliases, ", ")) + } + if cmd.Example != "" { + fmt.Fprintln(w, style(helpHeading, "Examples:")) + fmt.Fprintln(w, cmd.Example) + fmt.Fprintln(w) + } // Subcommands — rendered grouped when groups are defined, flat otherwise. cmds := cmd.Commands() @@ -144,18 +156,6 @@ func printHelp(w io.Writer, cmd *cobra.Command, color bool) { fmt.Fprintln(w) } - // Aliases - if len(cmd.Aliases) > 0 { - fmt.Fprintf(w, "%s %s\n\n", style(helpHeading, "Aliases:"), strings.Join(cmd.Aliases, ", ")) - } - - // Examples - if cmd.Example != "" { - fmt.Fprintln(w, style(helpHeading, "Examples:")) - fmt.Fprintln(w, cmd.Example) - fmt.Fprintln(w) - } - // Footer hint if cmd.HasAvailableSubCommands() { hint := `Use "` + cmd.CommandPath() + ` [command] --help" for more information about a command.` @@ -164,6 +164,21 @@ func printHelp(w io.Writer, cmd *cobra.Command, color bool) { } } +// usageLine renders the usage line, correcting it for command groups. +// +// UseLine() appends "[flags]" whenever a command has flags, so every group +// printed as "namecom domain [flags]" — an invocation that does nothing. A +// group's Use string is a bare name with no argument placeholders; when it also +// has subcommands, what it actually takes is one of them. The root command is +// left alone: "namecom [command] [flags]" is accurate there, since its +// persistent flags are the ones being documented. +func usageLine(cmd *cobra.Command) string { + if cmd.HasAvailableSubCommands() && cmd.HasParent() && !strings.Contains(cmd.Use, " ") { + return cmd.CommandPath() + " " + } + return cmd.UseLine() +} + // essentialGlobalFlagNames returns the subset of global flags shown on subcommand help pages. // Noisy flags (--debug, --timeout, --token, etc.) are omitted; users can run "namecom --help" // to see the full list. @@ -215,9 +230,16 @@ func printFlags(w io.Writer, fs *pflag.FlagSet, _ bool, style func(lipgloss.Styl if len(nameType) > maxLen { maxLen = len(nameType) } + // Quote string defaults, print everything else bare. Quoting + // uniformly rendered numbers as (default "1") and durations as + // (default "30s"), which reads like the flag wants a quoted literal. defVal := "" if f.DefValue != "" && f.DefValue != "false" && f.DefValue != "0" && f.DefValue != "0s" && f.DefValue != "[]" { - defVal = fmt.Sprintf(" (default %q)", f.DefValue) + if f.Value.Type() == "string" { + defVal = fmt.Sprintf(" (default %q)", f.DefValue) + } else { + defVal = fmt.Sprintf(" (default %s)", f.DefValue) + } } entries = append(entries, flagEntry{nameType: nameType, usage: f.Usage, defVal: defVal}) }) diff --git a/cmd/order/order.go b/cmd/order/order.go index d7ad726..112b1f0 100644 --- a/cmd/order/order.go +++ b/cmd/order/order.go @@ -75,6 +75,7 @@ func init() { _ = refundCmd.MarkFlagRequired("order-id") _ = refundCmd.MarkFlagRequired("item-ids") + cmdutil.GroupCmd(Cmd) Cmd.AddCommand(listCmd, getCmd, refundCmd) } diff --git a/cmd/root.go b/cmd/root.go index 8f3ec36..72ad7e6 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -45,6 +45,7 @@ type globalFlags struct { output string quiet bool noHeader bool + wide bool color string timeout time.Duration debug bool @@ -110,7 +111,7 @@ func Execute() { return cmdutil.NewUsageError(err) }) - if err := rootCmd.Execute(); err != nil { + if err := cmdutil.ClassifyCobraUsage(rootCmd.Execute()); err != nil { cfg := resolvedOut if cfg == nil { cfg = output.DefaultConfig() @@ -189,12 +190,13 @@ func init() { pf.StringVarP(&gf.output, "output", "o", "", "output format: table, json, yaml (default: table in TTY, json otherwise)") pf.BoolVarP(&gf.quiet, "quiet", "q", false, "print IDs/names only (one per line)") pf.BoolVar(&gf.noHeader, "no-header", false, "omit header row from table output") + pf.BoolVar(&gf.wide, "wide", false, "keep every table column even if it overflows the terminal") pf.StringVar(&gf.color, "color", "auto", "colorize output: auto, always, never (env: NO_COLOR, CLICOLOR_FORCE)") pf.DurationVar(&gf.timeout, "timeout", 30*time.Second, "per-request timeout") pf.BoolVar(&gf.debug, "debug", false, "log HTTP requests/responses to stderr (token redacted)") pf.StringVar(&gf.debugFile, "debug-file", "", "log HTTP requests/responses to this file instead of stderr") pf.BoolVarP(&gf.yes, "yes", "y", false, "skip confirmation prompts") - pf.BoolVar(&gf.dryRun, "dry-run", false, "print the API request that would be sent without executing it") + pf.BoolVar(&gf.dryRun, "dry-run", false, "for write operations, print the request instead of sending it (reads are unaffected)") pf.StringVar(&gf.idempKey, "idempotency-key", "", "idempotency key for write operations (auto-generated per invocation if not set)") pf.StringVar(&gf.baseURL, "base-url", "", "override the API base URL (for local stubs and proxies; credentials are sent to whatever you name)") @@ -221,7 +223,7 @@ func persistentPreRunE(cmd *cobra.Command, _ []string) error { return err } -// initOutputContext applies --output, --color, --quiet, and --no-header to the +// initOutputContext applies --output, --color, --quiet, --no-header, and --wide to the // command context. It runs for every command, including those that skip API // credential setup (auth, version, etc.). func initOutputContext(cmd *cobra.Command) error { @@ -244,6 +246,7 @@ func initOutputContext(cmd *cobra.Command) error { } out.QuietMode = gf.quiet out.NoHeader = gf.noHeader + out.Wide = gf.wide cmd.SetContext(context.WithValue(cmd.Context(), cmdutil.KeyOutput, out)) // Remember it for Execute's error path. That path ran before this config // existed and fell back to output.DefaultConfig(), which decides format by diff --git a/cmd/transfer/transfer.go b/cmd/transfer/transfer.go index 42ae225..86e31e1 100644 --- a/cmd/transfer/transfer.go +++ b/cmd/transfer/transfer.go @@ -113,6 +113,7 @@ func init() { listCmd.Flags().BoolVar(&listAll, "all", false, "fetch all pages (full transfer history)") + cmdutil.GroupCmd(Cmd) Cmd.AddCommand(listCmd, getCmd, createCmd, internalCmd, cancelCmd, cancelOutboundCmd, eligibilityCmd) } diff --git a/cmd/url/url.go b/cmd/url/url.go index 386b70a..cf0e938 100644 --- a/cmd/url/url.go +++ b/cmd/url/url.go @@ -97,6 +97,7 @@ func init() { updateCmd.Flags().StringVar(&updateTitle, "title", "", "page title (masked only)") updateCmd.Flags().StringVar(&updateMeta, "meta", "", "meta tags (masked only)") + cmdutil.GroupCmd(Cmd) Cmd.AddCommand(listCmd, getCmd, createCmd, updateCmd, deleteCmd) } diff --git a/cmd/vanity/vanity.go b/cmd/vanity/vanity.go index 0a8c12f..fb6aff5 100644 --- a/cmd/vanity/vanity.go +++ b/cmd/vanity/vanity.go @@ -83,6 +83,7 @@ func init() { updateCmd.Flags().StringVar(&updateIPs, "ips", "", "comma-separated IP addresses (required)") _ = updateCmd.MarkFlagRequired("ips") + cmdutil.GroupCmd(Cmd) Cmd.AddCommand(listCmd, getCmd, createCmd, updateCmd, deleteCmd) } diff --git a/internal/output/output.go b/internal/output/output.go index 265971f..52614a7 100644 --- a/internal/output/output.go +++ b/internal/output/output.go @@ -54,6 +54,11 @@ type Config struct { Writer io.Writer // defaults to os.Stdout EWriter io.Writer // defaults to os.Stderr Sandbox bool // true when targeting the sandbox API (--sandbox / profile) + Wide bool // --wide: never drop table columns, even if they overflow + // MaxWidth is the terminal width tables must fit inside. Zero means + // unconstrained, which is what a pipe or a redirect gets: a consumer that + // is not a terminal has no width to respect and wants every column. + MaxWidth int } // DefaultConfig returns an output config with defaults resolved from the @@ -63,12 +68,16 @@ func DefaultConfig() *Config { if isStdoutTTY() { f = FormatTable } - return &Config{ + c := &Config{ Format: f, Color: ColorAuto, Writer: os.Stdout, EWriter: os.Stderr, } + if w, _, err := term.GetSize(int(os.Stdout.Fd())); err == nil && w > 0 { + c.MaxWidth = w + } + return c } // IsInteractive reports whether stdin is a TTY — i.e., a human is present. @@ -206,8 +215,20 @@ func (c *Config) YAMLList(data any, nextPage *int32, total int32) error { } // Table renders rows as a styled table. headers is the column header row. +// Table renders headers and rows as a bordered table, dropping trailing +// columns that do not fit the terminal. +// +// Tables were rendered at their natural width with no regard for the terminal: +// `domain list` came to 113 columns, `order list` 99, `dns list` 87. In an +// 80-column terminal — an SSH session, a split pane — every one of them wrapped +// and the rounded borders came apart into unreadable fragments. Columns are +// ordered most- to least-important by their callers, so the ones that go are +// the ones from the right, and a footer names them rather than letting them +// vanish. --wide opts out; so does any non-terminal writer, since a pipe has no +// width to fit and its consumer wants the whole row. func (c *Config) Table(headers []string, rows [][]string) { color := c.ColorEnabled() + headers, rows, dropped := c.fitColumns(headers, rows) cell := lipgloss.NewStyle().Padding(0, 1) header := cell.Bold(color) @@ -239,6 +260,73 @@ func (c *Config) Table(headers []string, rows [][]string) { } fmt.Fprintln(c.Writer, t.Render()) + + if len(dropped) > 0 { + fmt.Fprintln(c.Writer, c.Dim(fmt.Sprintf( + "%d column%s hidden (%s) — widen the terminal, pass --wide, or use -o json", + len(dropped), plural("", len(dropped)), strings.Join(dropped, ", ")))) + } +} + +// fitColumns drops trailing columns until the rendered table fits MaxWidth, +// returning the surviving headers and rows plus the names of what went. +// +// The first column is never dropped: a table of nothing but a row count is +// worse than one that overflows, and callers put the identifying column first. +func (c *Config) fitColumns(headers []string, rows [][]string) ([]string, [][]string, []string) { + if c.Wide || c.MaxWidth <= 0 || len(headers) == 0 { + return headers, rows, nil + } + + keep := len(headers) + for keep > 1 && tableWidth(colWidths(headers, rows, keep)) > c.MaxWidth { + keep-- + } + if keep == len(headers) { + return headers, rows, nil + } + + dropped := make([]string, 0, len(headers)-keep) + for _, h := range headers[keep:] { + dropped = append(dropped, strings.ToLower(h)) + } + trimmed := make([][]string, 0, len(rows)) + for _, r := range rows { + if len(r) > keep { + r = r[:keep] + } + trimmed = append(trimmed, r) + } + return headers[:keep], trimmed, dropped +} + +// colWidths measures the first n columns at their natural (widest-cell) width. +// lipgloss.Width is used rather than len because cells arrive pre-styled — +// ExpiryDate returns ANSI escapes — and because a domain may hold wide runes. +func colWidths(headers []string, rows [][]string, n int) []int { + w := make([]int, n) + for i := 0; i < n && i < len(headers); i++ { + w[i] = lipgloss.Width(headers[i]) + } + for _, r := range rows { + for i := 0; i < n && i < len(r); i++ { + if cw := lipgloss.Width(r[i]); cw > w[i] { + w[i] = cw + } + } + } + return w +} + +// tableWidth is the rendered width of a table with the given column widths: +// each column carries one space of padding on each side, and there is a border +// rune before the first column, between every pair, and after the last. +func tableWidth(widths []int) int { + total := len(widths) + 1 + for _, w := range widths { + total += w + 2 + } + return total } // KVTable renders a headerless two-column key-value table with styled field names. @@ -503,9 +591,7 @@ func (c *Config) ExpiryDate(t *time.Time) string { return label } switch { - case days < 0: - return lipgloss.NewStyle().Bold(true).Foreground(acRed).Render(label) - case days < 7: + case days < 7: // expired, or expiring inside a week return lipgloss.NewStyle().Bold(true).Foreground(acRed).Render(label) case days < 30: return lipgloss.NewStyle().Bold(true).Foreground(acAmber).Render(label) @@ -514,7 +600,15 @@ func (c *Config) ExpiryDate(t *time.Time) string { } } -// relativeTime converts a floating-point day count into a human-readable string. +// relativeTime converts a floating-point day count into a human-readable +// string, widening the unit as the distance grows. +// +// It used to speak only days, which is right near an expiry and useless far +// from one: a domain paid through 2034 rendered as "in 2750 days", a number no +// reader converts to anything meaningful. Days stay exact inside a quarter, +// where renewal decisions actually happen; past that the unit widens. The +// absolute date sits immediately before this string in every caller, so the +// parenthetical only has to convey magnitude. func relativeTime(days float64) string { abs := days if abs < 0 { @@ -523,21 +617,34 @@ func relativeTime(days float64) string { switch { case days < 0 && abs < 1: return "expired today" - case days < 0: - n := int(abs + 0.5) - if n == 1 { - return "1 day ago" - } - return fmt.Sprintf("%d days ago", n) - case days < 1: + case days < 1 && days >= 0: return "today" + } + unit, n := humanizeDays(abs) + if days < 0 { + return fmt.Sprintf("%d %s ago", n, plural(unit, n)) + } + return fmt.Sprintf("in %d %s", n, plural(unit, n)) +} + +// humanizeDays picks the coarsest unit that still says something useful about a +// span, and returns the count in that unit. +func humanizeDays(abs float64) (unit string, n int) { + switch { + case abs <= 90: + return "day", int(abs + 0.5) + case abs < 730: + return "month", int(abs/30.44 + 0.5) default: - n := int(days + 0.5) - if n == 1 { - return "in 1 day" - } - return fmt.Sprintf("in %d days", n) + return "year", int(abs/365.25 + 0.5) + } +} + +func plural(unit string, n int) string { + if n == 1 { + return unit } + return unit + "s" } // spinFrames are the animation frames for the spinner. diff --git a/internal/output/output_test.go b/internal/output/output_test.go index d5b0cc4..7df7f1f 100644 --- a/internal/output/output_test.go +++ b/internal/output/output_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/charmbracelet/lipgloss" "gopkg.in/yaml.v3" ) @@ -707,3 +708,115 @@ func TestTTYPredicates_ReportNonTTYUnderTest(t *testing.T) { t.Error("IsInteractive() = true under `go test`, where stdin is not a terminal") } } + +// TestRelativeTimeWidensUnit guards the unit-widening thresholds. Day counts +// are exact inside a quarter, where a renewal decision is actually pending; +// past that they widen, because "in 2750 days" told a reader nothing about a +// domain paid through 2034. +func TestRelativeTimeWidensUnit(t *testing.T) { + tests := []struct { + days float64 + want string + }{ + {0.5, "today"}, + {-0.5, "expired today"}, + {1, "in 1 day"}, + {90, "in 90 days"}, // boundary: still exact days + {91, "in 3 months"}, // first step up + {194, "in 6 months"}, // a real expiry from `domain list` + {729, "in 24 months"}, + {730, "in 2 years"}, // boundary: months give way to years + {2750, "in 8 years"}, + {-3, "3 days ago"}, + {-1, "1 day ago"}, + {-758, "2 years ago"}, // a real expired domain from `domain list` + } + for _, tt := range tests { + if got := relativeTime(tt.days); got != tt.want { + t.Errorf("relativeTime(%.1f) = %q, want %q", tt.days, got, tt.want) + } + } +} + +// TestTableFitsTerminalWidth guards the column-dropping behavior. Tables were +// rendered at natural width regardless of the terminal: `domain list` measured +// 113 columns against an 80-column pane, and the rounded borders came apart on +// wrap. Columns now drop from the right, which is the least-important end, +// and the footer names what went so nothing disappears silently. +func TestTableFitsTerminalWidth(t *testing.T) { + headers := []string{"DOMAIN", "EXPIRES", "AUTO-RENEW", "LOCKED", "PRIVACY"} + rows := [][]string{ + {"loadtest-ff7fb52b-b51b-46c8-b254-6a557f053321.com", "2027-03-01 (in 6 months)", "yes", "yes", "no"}, + {"beers.army", "2034-03-01 (in 8 years)", "yes", "yes", "yes"}, + } + + widest := func(s string) int { + widest := 0 + for _, line := range strings.Split(strings.TrimRight(s, "\n"), "\n") { + if w := lipgloss.Width(line); w > widest { + widest = w + } + } + return widest + } + + t.Run("drops columns to fit", func(t *testing.T) { + var buf bytes.Buffer + c := &Config{Format: FormatTable, Writer: &buf, EWriter: &buf, MaxWidth: 80} + c.Table(headers, rows) + got := buf.String() + + // The footer names hidden columns; measure only the table itself. + var tableLines []string + for _, line := range strings.Split(got, "\n") { + if strings.ContainsAny(line, "│╭╰├") { + tableLines = append(tableLines, line) + } + } + if w := widest(strings.Join(tableLines, "\n")); w > 80 { + t.Errorf("table rendered %d columns wide, want <= 80:\n%s", w, got) + } + if !strings.Contains(got, "hidden") { + t.Errorf("dropped columns without telling the reader:\n%s", got) + } + if !strings.Contains(got, "DOMAIN") { + t.Errorf("dropped the identifying column:\n%s", got) + } + }) + + t.Run("--wide keeps every column", func(t *testing.T) { + var buf bytes.Buffer + c := &Config{Format: FormatTable, Writer: &buf, EWriter: &buf, MaxWidth: 80, Wide: true} + c.Table(headers, rows) + got := buf.String() + for _, h := range headers { + if !strings.Contains(got, h) { + t.Errorf("--wide dropped %q:\n%s", h, got) + } + } + if strings.Contains(got, "hidden") { + t.Errorf("--wide should not print a hidden-column footer:\n%s", got) + } + }) + + t.Run("no width constraint keeps every column", func(t *testing.T) { + var buf bytes.Buffer + c := &Config{Format: FormatTable, Writer: &buf, EWriter: &buf} // MaxWidth 0: piped + c.Table(headers, rows) + got := buf.String() + for _, h := range headers { + if !strings.Contains(got, h) { + t.Errorf("piped output dropped %q:\n%s", h, got) + } + } + }) + + t.Run("wide enough drops nothing", func(t *testing.T) { + var buf bytes.Buffer + c := &Config{Format: FormatTable, Writer: &buf, EWriter: &buf, MaxWidth: 200} + c.Table(headers, rows) + if got := buf.String(); strings.Contains(got, "hidden") { + t.Errorf("dropped columns that fit:\n%s", got) + } + }) +} From 72970056cd7ed21626c617697abcebe23d47cba2 Mon Sep 17 00:00:00 2001 From: patramsey Date: Wed, 19 Aug 2026 18:41:08 -0600 Subject: [PATCH 2/2] test: cover the help-layout and expiry-threshold fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codecov put patch coverage at 89.6%. Most of the miss was pre-existing colour branches and Execute's os.Exit path, but three of the fixes in this branch genuinely had no test: the Examples/Flags ordering, the command-group usage line, and the non-string default formatting. All three are the kind of thing that reverts silently when someone edits the template for an unrelated reason. The expiry-threshold test is the reason for the small refactor. Written the obvious way it asserted on rendered ANSI prefixes — and lipgloss degrades every style to a no-op off a TTY, so each expected prefix was the empty string and all three cases passed without checking anything. expiryStyle is split out of ExpiryDate so the thresholds can be compared as values instead. Every new assertion was mutation-checked: reverting the week threshold, the unit-widening threshold, the column dropping, the group usage line, the default quoting, the required-flag prefix, the Examples ordering, and the unknown-subcommand rejection each fail at least one test. --- cmd/cmdutil/args_test.go | 13 ++++++ cmd/help_test.go | 84 ++++++++++++++++++++++++++++++++++ internal/output/output.go | 15 ++++-- internal/output/output_test.go | 44 ++++++++++++++++++ 4 files changed, 153 insertions(+), 3 deletions(-) diff --git a/cmd/cmdutil/args_test.go b/cmd/cmdutil/args_test.go index 8c689b0..b243d6f 100644 --- a/cmd/cmdutil/args_test.go +++ b/cmd/cmdutil/args_test.go @@ -1,6 +1,7 @@ package cmdutil import ( + "bytes" "errors" "strings" "testing" @@ -205,6 +206,18 @@ func TestGroupCmd(t *testing.T) { } }) + t.Run("bare group prints help rather than erroring", func(t *testing.T) { + group := newGroup() + var buf bytes.Buffer + group.SetOut(&buf) + if err := group.RunE(group, nil); err != nil { + t.Fatalf("group RunE returned an error: %v", err) + } + if !strings.Contains(buf.String(), "register") { + t.Errorf("bare group did not print its subcommands:\n%s", buf.String()) + } + }) + t.Run("suggestion distance is set", func(t *testing.T) { // Cobra defaults this to 2 only inside the root-command path being // replaced here. Left at zero, SuggestionsFor matches nothing. diff --git a/cmd/help_test.go b/cmd/help_test.go index 81c9059..54ae8c8 100644 --- a/cmd/help_test.go +++ b/cmd/help_test.go @@ -5,6 +5,7 @@ import ( "regexp" "strings" "testing" + "time" "github.com/charmbracelet/lipgloss" "github.com/spf13/cobra" @@ -193,3 +194,86 @@ func TestPrintFilteredFlags_ShowsOnlyAllowedFlags(t *testing.T) { t.Errorf("a flag outside the allow list must not be rendered, got:\n%s", got) } } + +// TestHelpLayout pins the three help-page fixes. None of them had a test, and +// all three are the kind of thing that silently reverts when someone edits the +// template for an unrelated reason. +func TestHelpLayout(t *testing.T) { + noop := func(*cobra.Command, []string) {} + + t.Run("examples appear above the flag tables", func(t *testing.T) { + // Examples used to print last — below Flags, below Global Flags, and + // below the "see all global options" footer — which put the most-read + // part of a help page furthest down it. + root := &cobra.Command{Use: "namecom"} + cmd := &cobra.Command{ + Use: "create ", + Short: "Create a DNS record", + Example: " namecom dns create example.com --type A --answer 1.2.3.4", + Run: noop, + } + cmd.Flags().String("answer", "", "record value") + root.AddCommand(cmd) + + var buf bytes.Buffer + printHelp(&buf, cmd, false) + got := buf.String() + + examples := strings.Index(got, "Examples:") + flags := strings.Index(got, "Flags:") + if examples < 0 || flags < 0 { + t.Fatalf("help is missing a section:\n%s", got) + } + if examples > flags { + t.Errorf("Examples renders below Flags:\n%s", got) + } + }) + + t.Run("a command group asks for a subcommand, not flags", func(t *testing.T) { + // UseLine() appends "[flags]" to anything with flags, so every group + // advertised `namecom domain [flags]` — an invocation that does nothing. + root := &cobra.Command{Use: "namecom"} + group := &cobra.Command{Use: "domain", Short: "Manage domains"} + group.AddCommand(&cobra.Command{Use: "list", Short: "List domains", Run: noop}) + root.AddCommand(group) + + if got := usageLine(group); got != "namecom domain " { + t.Errorf("usageLine(group) = %q, want %q", got, "namecom domain ") + } + // A leaf command keeps cobra's own usage line, placeholders and all. + leaf := &cobra.Command{Use: "get ", Run: noop} + root.AddCommand(leaf) + if got := usageLine(leaf); !strings.Contains(got, "") { + t.Errorf("usageLine(leaf) = %q, want it to keep its argument placeholder", got) + } + // The root keeps "[command] [flags]": its persistent flags are the + // ones the page is documenting. + if got := usageLine(root); strings.Contains(got, "") { + t.Errorf("usageLine(root) = %q, want cobra's own line", got) + } + }) + + t.Run("only string defaults are quoted", func(t *testing.T) { + // Quoting uniformly rendered (default "300") and (default "30s"), + // which reads like the flag wants a quoted literal. + fs := pflag.NewFlagSet("test", pflag.ContinueOnError) + fs.String("host", "@", "hostname") + fs.Int64("ttl", 300, "TTL in seconds") + fs.Duration("timeout", 30*time.Second, "per-request timeout") + + var buf bytes.Buffer + printFlags(&buf, fs, false, noStyle) + got := buf.String() + + for _, want := range []string{`(default "@")`, `(default 300)`, `(default 30s)`} { + if !strings.Contains(got, want) { + t.Errorf("missing %s in:\n%s", want, got) + } + } + for _, unwanted := range []string{`(default "300")`, `(default "30s")`} { + if strings.Contains(got, unwanted) { + t.Errorf("non-string default rendered as %s:\n%s", unwanted, got) + } + } + }) +} diff --git a/internal/output/output.go b/internal/output/output.go index 52614a7..04fca51 100644 --- a/internal/output/output.go +++ b/internal/output/output.go @@ -590,13 +590,22 @@ func (c *Config) ExpiryDate(t *time.Time) string { if !c.ColorEnabled() { return label } + return expiryStyle(days).Render(label) +} + +// expiryStyle picks the urgency styling for a date this many days away. +// +// Split out from ExpiryDate so the thresholds can be asserted directly: off a +// TTY lipgloss degrades every style to a no-op, so a test comparing rendered +// output cannot tell red from dim and passes whatever the code does. +func expiryStyle(days float64) lipgloss.Style { switch { case days < 7: // expired, or expiring inside a week - return lipgloss.NewStyle().Bold(true).Foreground(acRed).Render(label) + return lipgloss.NewStyle().Bold(true).Foreground(acRed) case days < 30: - return lipgloss.NewStyle().Bold(true).Foreground(acAmber).Render(label) + return lipgloss.NewStyle().Bold(true).Foreground(acAmber) default: - return styleDim.Render(label) + return styleDim } } diff --git a/internal/output/output_test.go b/internal/output/output_test.go index 7df7f1f..e421a03 100644 --- a/internal/output/output_test.go +++ b/internal/output/output_test.go @@ -820,3 +820,47 @@ func TestTableFitsTerminalWidth(t *testing.T) { } }) } + +// TestExpiryStyleThresholds pins the urgency thresholds. The expired and +// expiring-this-week cases were two byte-identical switch arms; merging them is +// only safe if something asserts both still resolve to red. +// +// It asserts on the style rather than on rendered output because lipgloss +// degrades to no-op styles off a TTY: an earlier version of this test compared +// ANSI prefixes, every expected prefix was the empty string, and all three +// cases passed without checking anything. +func TestExpiryStyleThresholds(t *testing.T) { + tests := []struct { + name string + days float64 + want lipgloss.TerminalColor + }{ + {"long expired", -758, acRed}, + {"expired today", -0.5, acRed}, + {"expiring inside a week", 3, acRed}, + {"the week boundary", 6.9, acRed}, + {"expiring inside a month", 20, acAmber}, + {"the month boundary", 29.9, acAmber}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := expiryStyle(tt.days).GetForeground(); got != tt.want { + t.Errorf("expiryStyle(%.1f) foreground = %v, want %v", tt.days, got, tt.want) + } + }) + } + + t.Run("far future is neither red nor amber", func(t *testing.T) { + fg := expiryStyle(900).GetForeground() + if fg == acRed || fg == acAmber { + t.Errorf("expiryStyle(900) = %v, want the dim style", fg) + } + }) + + t.Run("a far-future date still reads in years", func(t *testing.T) { + at := time.Now().Add(900 * 24 * time.Hour) + if got := noColor().ExpiryDate(&at); !strings.Contains(got, "in 2 years") { + t.Errorf("ExpiryDate(900 days) = %q, want a year-scale relative time", got) + } + }) +}