Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions PLAN.md
Original file line number Diff line number Diff line change
Expand Up @@ -440,8 +440,9 @@ Groups are the opposite case: `Command::get_groups`, `ArgGroup::get_args` and
- [ ] **The full `PossibleValue` model** — `ignore_case`, aliases and their
visibility, per-value help, and hidden values. Subcommand variants take
aliases. The KDL model, usage-lib parser, and clap bridge now preserve this
metadata; generated Rust and Go tables still flatten choices to strings and are
the remaining work before this item is complete.
metadata. Generated Go tables now distinguish accepted aliases and hidden values
from the visible diagnostic/help list and honor `ignore_case`; generated Rust
tables are the remaining work before this item is complete.
- [ ] **`infer_subcommands` / `infer_long_args`** — unambiguous prefixes. We
suggest what was probably meant but do not accept it.
- [x] **`external_subcommand`** — an unmatched word is forwarded with the rest
Expand Down
23 changes: 16 additions & 7 deletions go/argv/post.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package argv

import "os"
import (
"os"
"strings"
)

// The rules that are decided once the last token has been read.
//
Expand Down Expand Up @@ -57,9 +60,11 @@ type Meta struct {
RequiresIfBoolean bool
// Required means it must end up with a value, from anywhere.
Required bool
// Choices is the exact set of values allowed. Matching is case-sensitive:
// case-insensitive matching would have to be declared rather than assumed.
// Choices is the visible set shown in diagnostics.
Choices []string
// AcceptedChoices also includes hidden values and aliases.
AcceptedChoices []string
IgnoreCase bool
// Default fills in when neither the command line nor the environment did.
Default []string
// Env names an environment variable to fall back to. Empty means none.
Expand Down Expand Up @@ -291,11 +296,15 @@ func Check(m *Meta, values []string, occurrences int) *Error {
return &Error{Code: code, Name: m.Name, Spelling: m.Spelling}
}

if len(m.Choices) > 0 {
accepted := m.AcceptedChoices
if len(accepted) == 0 {
accepted = m.Choices
}
if len(accepted) > 0 {
// Every value, not just the first: a variadic can be given a good value
// and a bad one, and so can a repeatable flag across occurrences.
for _, v := range values {
if !contains(m.Choices, v) {
if !containsChoice(accepted, v, m.IgnoreCase) {
return &Error{Code: CodeInvalidChoice, Name: m.Name,
Spelling: m.Spelling, Choices: m.Choices}
}
Expand Down Expand Up @@ -335,9 +344,9 @@ func EnvTruth(value string) bool {
return false
}

func contains(list []string, s string) bool {
func containsChoice(list []string, s string, ignoreCase bool) bool {
for _, x := range list {
if x == s {
if x == s || ignoreCase && strings.EqualFold(x, s) {
return true
}
}
Expand Down
17 changes: 17 additions & 0 deletions go/argv/post_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,23 @@ func TestCheck(t *testing.T) {
}
}

func TestCheckRichChoices(t *testing.T) {
meta := &Meta{
Name: "color",
Choices: []string{"always"},
AcceptedChoices: []string{"always", "never", "yes"},
IgnoreCase: true,
}
for _, value := range []string{"ALWAYS", "NEVER", "YES"} {
if err := Check(meta, []string{value}, 1); err != nil {
t.Errorf("%q should be accepted: %v", value, err)
}
}
if err := Check(meta, []string{"sometimes"}, 1); err == nil || err.Code != CodeInvalidChoice {
t.Fatalf("an unknown value should be rejected: %+v", err)
}
}

// TestEnvTruth pins the allow-list, including what it deliberately leaves out.
func TestEnvTruth(t *testing.T) {
for _, s := range []string{"1", "true", "True", "TRUE"} {
Expand Down
82 changes: 70 additions & 12 deletions go/internal/spec/spec.go
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,14 @@ func (f *Flag) choices() []string {
if f.Arg == nil {
return nil
}
return f.Arg.Choices.list()
return f.Arg.Choices.visible()
}

func (f *Flag) acceptedChoices() []string {
if f.Arg == nil {
return nil
}
return f.Arg.Choices.accepted()
}

// defaults reads through to the value a flag takes, which is the other place a
Expand Down Expand Up @@ -309,14 +316,61 @@ type Example struct {

// Choices is the declared set of values, which the lowering nests one level.
type Choices struct {
Choices []string `json:"choices"`
Choices []string `json:"choices"`
Details []Choice `json:"details"`
IgnoreCase bool `json:"ignore_case"`
}

type Choice struct {
Value string `json:"value"`
Help string `json:"help"`
Hide bool `json:"hide"`
Aliases []ChoiceAlias `json:"aliases"`
}

type ChoiceAlias struct {
Value string `json:"value"`
Hide bool `json:"hide"`
}

func (c *Choices) accepted() []string {
if c == nil {
return nil
}
out := append([]string(nil), c.Choices...)
for _, detail := range c.Details {
for _, alias := range detail.Aliases {
out = append(out, alias.Value)
}
}
return out
}

func (c *Choices) list() []string {
func (c *Choices) visible() []string {
if c == nil {
return nil
}
return c.Choices
out := make([]string, 0, len(c.Choices))
for _, value := range c.Choices {
hidden := false
for _, detail := range c.Details {
if detail.Value == value && detail.Hide {
hidden = true
break
}
}
if !hidden {
out = append(out, value)
}
}
for _, detail := range c.Details {
for _, alias := range detail.Aliases {
if !alias.Hide {
out = append(out, alias.Value)
}
}
}
return out
}

// Multi is how a flag accumulates when it is given more than once.
Expand Down Expand Up @@ -765,6 +819,8 @@ func (b *builder) flag(f *Flag) *argv.Flag {
CompleteType: b.completeType(first(valueOf(f), f.Name)),
Required: f.Required,
Choices: f.choices(),
AcceptedChoices: f.acceptedChoices(),
IgnoreCase: f.Arg != nil && f.Arg.Choices != nil && f.Arg.Choices.IgnoreCase,
Default: f.defaults(),
Env: f.Env,
VarMin: clampVarMax(f.VarMin),
Expand Down Expand Up @@ -802,18 +858,20 @@ func (b *builder) arg(a *Arg) *argv.Arg {
Short: first(a.Help, a.HelpFirstLine),
Long: first(a.HelpLong, a.Help),
Heading: a.HelpHeading,
Choices: a.Choices.list(),
Choices: a.Choices.visible(),
Env: a.Env,
Default: a.Default,
})
b.record(out.Key, argv.Meta{
Name: a.Name,
Required: a.Required,
CompleteType: b.completeType(a.Name),
Choices: a.Choices.list(),
Default: a.Default,
Env: a.Env,
VarMin: clampVarMax(a.VarMin),
Name: a.Name,
Required: a.Required,
CompleteType: b.completeType(a.Name),
Choices: a.Choices.visible(),
AcceptedChoices: a.Choices.accepted(),
Comment thread
cursor[bot] marked this conversation as resolved.
Resolved
IgnoreCase: a.Choices != nil && a.Choices.IgnoreCase,
Default: a.Default,
Env: a.Env,
VarMin: clampVarMax(a.VarMin),
// No VarMax: for an argument the bound is a limit binding applies, which
// is what makes `[a]… [b]` fillable at all, so judging it again here would
// fail an invocation that never broke it.
Expand Down
25 changes: 25 additions & 0 deletions go/internal/spec/spec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,31 @@ func TestChoicesAreReadThroughTheValue(t *testing.T) {
}
}

func TestRichChoicesSeparateAcceptanceFromVisibility(t *testing.T) {
choices := &Choices{
Choices: []string{"always", "never"},
IgnoreCase: true,
Details: []Choice{
{Value: "always", Aliases: []ChoiceAlias{{Value: "yes"}, {Value: "on", Hide: true}}},
{Value: "never", Hide: true},
},
}
root, meta := build(&Spec{
Name: "ex", Bin: "ex",
Cmd: Cmd{Name: "ex", Args: []Arg{{Name: "color", Choices: choices}}},
})
entry := metaFor(t, meta, root, "color")
if !entry.IgnoreCase {
t.Error("ignore_case was lost")
}
if !reflect.DeepEqual(entry.Choices, []string{"always", "yes"}) {
t.Errorf("visible choices: %q", entry.Choices)
}
if !reflect.DeepEqual(entry.AcceptedChoices, []string{"always", "never", "yes", "on"}) {
t.Errorf("accepted choices: %q", entry.AcceptedChoices)
}
}

// The two tables are separate data tied together by key, so the tie is what is
// worth testing: every entry's metadata must describe that entry and no other.
func TestMetadataLinesUpWithTheParseTables(t *testing.T) {
Expand Down
Loading