diff --git a/cmd/morphic/command.go b/cmd/morphic/command.go index 0fe8af0..ddafa02 100644 --- a/cmd/morphic/command.go +++ b/cmd/morphic/command.go @@ -1,9 +1,6 @@ package main -import ( - "flag" - "io" -) +import "io" // command describes one morphic subcommand: how it is invoked, how it is // documented, and how it runs. Dispatch and help rendering both read this @@ -17,21 +14,34 @@ type command struct { usage string // description is the paragraph shown above the flag table in command help. description string - // flagSet returns a fresh FlagSet with this command's flags defined. Help - // rendering and argument parsing share it, so the flag table printed by - // PrintDefaults cannot drift from what Parse accepts. - flagSet func() *flag.FlagSet + // printFlags writes this command's flag table to w. It renders from the same + // constructor that parsing binds, so the documented flags cannot drift from + // the flags Parse accepts. + // + // It hands out the rendered text rather than the *flag.FlagSet because help + // needs nothing more: a FlagSet in a caller's hands can be Parsed, and that + // writes into an options struct nobody is holding, losing the values with no + // error to notice. + printFlags func(w io.Writer) // run executes the command with the subcommand word already removed from // args, and returns the process exit code. run func(args []string, stdout, stderr io.Writer) int } -// commands is the subcommand table. Adding a subcommand means adding one entry. -var commands = []command{compileCommand} +// commands returns the subcommand table. Adding a subcommand means adding one +// entry. +// +// It is a function rather than a var so that a command's own code may reach +// back into the table — writeRootHelp already does, and any error path that +// wants to list the commands will too. As a var it sits in the initialization +// graph, and the first such reference is an initialization cycle +// (commands → newCompileCommand → runCompile → writeRootHelp → commands) that +// stops the package compiling. Functions may freely refer to each other. +func commands() []command { return []command{newCompileCommand()} } // lookup resolves a subcommand by name. func lookup(name string) (command, bool) { - for _, c := range commands { + for _, c := range commands() { if c.name == name { return c, true } diff --git a/cmd/morphic/command_test.go b/cmd/morphic/command_test.go index 11be2d1..a94b45b 100644 --- a/cmd/morphic/command_test.go +++ b/cmd/morphic/command_test.go @@ -1,7 +1,9 @@ package main import ( + "bytes" "flag" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -17,7 +19,7 @@ func TestLookup_KnownAndUnknown(t *testing.T) { assert.NotEmpty(t, c.summary, "every command needs a summary for the root help list") assert.NotEmpty(t, c.usage) assert.NotEmpty(t, c.description) - require.NotNil(t, c.flagSet) + require.NotNil(t, c.printFlags) require.NotNil(t, c.run) _, ok = lookup("bogus") @@ -34,9 +36,10 @@ func TestLookup_KnownAndUnknown(t *testing.T) { func TestCommands_TableIsWellFormed(t *testing.T) { t.Parallel() - require.NotEmpty(t, commands, "the command table must not be empty") - seen := make(map[string]bool, len(commands)) - for _, c := range commands { + table := commands() + require.NotEmpty(t, table, "the command table must not be empty") + seen := make(map[string]bool, len(table)) + for _, c := range table { require.NotEmpty(t, c.name, "every command needs a name") require.False(t, seen[c.name], "duplicate command %q", c.name) seen[c.name] = true @@ -64,19 +67,35 @@ func TestNewCompileFlags_DefinesEveryFlag(t *testing.T) { // constructor and the command-table entry so the two cannot drift. var compileFlagNames = []string{"o", "fail-on", "skip-validate", "explain"} -func TestCommand_FlagSetBindsTheCommandsOwnFlags(t *testing.T) { +func TestCommand_PrintFlagsDocumentsTheCommandsOwnFlags(t *testing.T) { t.Parallel() c, ok := lookup("compile") require.True(t, ok) + require.NotNil(t, c.printFlags) - // The table's flagSet is what help rendering reads. Today it just calls - // newCompileFlags, so nothing can drift yet; this holds the line if the - // table ever gets its own. - fs := c.flagSet() - require.NotNil(t, fs) + var buf bytes.Buffer + c.printFlags(&buf) + require.NotEmpty(t, buf.String(), "printFlags must render something") - var got []string - fs.VisitAll(func(f *flag.Flag) { got = append(got, f.Name) }) - assert.ElementsMatch(t, compileFlagNames, got) + // Read back out of the render rather than off a FlagSet: the render is what + // a user sees, and it is all the table hands out. + assert.ElementsMatch(t, compileFlagNames, flagNamesIn(buf.String()), + "the rendered flag table must document exactly the flags compile accepts") +} + +// flagNamesIn returns the flag names documented by a rendered flag table. +// PrintDefaults writes each flag as " -name ..." above an indented description +// line, so a line carrying the " -" prefix names a flag and nothing else does. +func flagNamesIn(rendered string) []string { + var names []string + for _, line := range strings.Split(rendered, "\n") { + rest, ok := strings.CutPrefix(line, " -") + if !ok { + continue + } + name, _, _ := strings.Cut(rest, " ") + names = append(names, name) + } + return names } diff --git a/cmd/morphic/compile.go b/cmd/morphic/compile.go index b8fe36f..e63a142 100644 --- a/cmd/morphic/compile.go +++ b/cmd/morphic/compile.go @@ -3,6 +3,7 @@ package main import ( "context" "encoding/json" + "errors" "flag" "fmt" "io" @@ -24,18 +25,28 @@ var newEngine = engine.New // not fail after a successful write on the platforms Morphic targets. var openOutput = func(path string) (io.WriteCloser, error) { return os.Create(path) } -// compileCommand is compile's entry in the command table. -var compileCommand = command{ - name: "compile", - summary: "lower an API spec (OpenAPI 3.x) into Morphic IR JSON", - usage: "morphic compile [flags]", - description: "Lower an API spec (OpenAPI 3.x) into Morphic IR JSON on stdout, and write\n" + - "diagnostics to stderr.", - flagSet: func() *flag.FlagSet { - fs, _ := newCompileFlags() - return fs - }, - run: runCompile, +// newCompileCommand builds compile's command-table entry. It is a function, +// not a package-level var, because its run field refers to runCompile, and +// runCompile reads this same metadata back to render help and usage text. See +// commands for why every step of that loop has to stay out of the +// initialization graph. +func newCompileCommand() command { + return command{ + name: "compile", + summary: "lower an API spec (OpenAPI 3.x) into Morphic IR JSON", + usage: "morphic compile [flags]", + description: "Lower an API spec (OpenAPI 3.x) into Morphic IR JSON on stdout, and write\n" + + "diagnostics to stderr.\n\n" + + "--explain reports what compiling produced at one source coordinate — the\n" + + "type node interned there, the coordinates interned beneath it, and the\n" + + "diagnostics stamped at it — instead of writing the document.", + printFlags: func(w io.Writer) { + fs, _ := newCompileFlags() + fs.SetOutput(w) + fs.PrintDefaults() + }, + run: runCompile, + } } // compileOptions holds the values compile's flags parse into. @@ -73,25 +84,36 @@ func runCompile(args []string, stdout, stderr io.Writer) int { fs, opts := newCompileFlags() positional, err := parseArgs(fs, args) + if errors.Is(err, flag.ErrHelp) { + writeCommandHelp(stdout, newCompileCommand()) + return 0 + } if err != nil { - emitf(stderr, "morphic: %v\n", err) - printUsage(stderr) - return 2 + return compileUsageError(stderr, err.Error()) } if opts.failOn != "error" && opts.failOn != "warning" { - emitf(stderr, "morphic: invalid --fail-on %q (want error or warning)\n", opts.failOn) - printUsage(stderr) - return 2 + return compileUsageError(stderr, + fmt.Sprintf("invalid --fail-on %q (want error or warning)", opts.failOn)) } if len(positional) != 1 { - emitf(stderr, "morphic: compile requires exactly one spec file\n") - printUsage(stderr) - return 2 + return compileUsageError(stderr, "compile requires exactly one spec file") } return compileSpec(positional[0], *opts, stdout, stderr) } +// compileUsageError reports a misuse of compile: one reason line, one short +// usage pointer, exit 2. It never touches stdout. +// +// reason is a finished string, not a format: a printf-style wrapper here is +// invisible to vet, so a reason carrying a literal % — a flag value echoed back +// from the user, say — would be mangled into the output with nothing to catch it. +func compileUsageError(stderr io.Writer, reason string) int { + emitf(stderr, "morphic: %s\n", reason) + writeCommandUsage(stderr, newCompileCommand()) + return 2 +} + // compileSpec runs the pipeline over specPath, writes the IR document and its // diagnostics, and returns the process exit code. func compileSpec(specPath string, opts compileOptions, stdout, stderr io.Writer) int { diff --git a/cmd/morphic/compile_test.go b/cmd/morphic/compile_test.go index 56d26df..0ec7157 100644 --- a/cmd/morphic/compile_test.go +++ b/cmd/morphic/compile_test.go @@ -75,9 +75,36 @@ components: func TestRun_UsageErrors(t *testing.T) { t.Parallel() - var stdout, stderr bytes.Buffer - assert.Equal(t, 2, run(nil, &stdout, &stderr)) - assert.Equal(t, 2, run([]string{"bogus"}, &stdout, &stderr)) - assert.Equal(t, 2, run([]string{"compile", "x.yaml", "--fail-on", "hint"}, &stdout, &stderr)) - assert.True(t, strings.Contains(stderr.String(), "usage")) + + spec := writeFile(t, "spec.yaml", testspec.Tiny) + tests := []struct { + name string + args []string + reason string + }{ + {"unknown command", []string{"bogus"}, `unknown command "bogus"`}, + {"help of unknown command", []string{"help", "bogus"}, `unknown command "bogus"`}, + {"help of unknown command with help flag", []string{"help", "bogus", "--help"}, `unknown command "bogus"`}, + {"help flag of unknown command", []string{"-h", "bogus"}, `unknown command "bogus"`}, + {"help with extra args", []string{"help", "compile", "extra"}, "help accepts at most one command"}, + {"help flag with extra args", []string{"--help", "compile", "extra"}, "help accepts at most one command"}, + {"help with extra args and help flag", []string{"help", "a", "b", "--help"}, "help accepts at most one command"}, + {"unknown flag", []string{"compile", spec, "--bogus"}, "flag provided but not defined: -bogus"}, + {"no spec file", []string{"compile"}, "compile requires exactly one spec file"}, + {"two spec files", []string{"compile", spec, spec}, "compile requires exactly one spec file"}, + {"bad fail-on", []string{"compile", spec, "--fail-on", "hint"}, `invalid --fail-on "hint"`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + var stdout, stderr bytes.Buffer + + assert.Equal(t, 2, run(tt.args, &stdout, &stderr)) + assert.Empty(t, stdout.String(), "usage errors must never write to stdout") + assert.Contains(t, stderr.String(), tt.reason) + assert.Equal(t, 1, strings.Count(stderr.String(), "usage:"), + "exactly one usage block per misuse, got:\n%s", stderr.String()) + }) + } } diff --git a/cmd/morphic/edgecases_test.go b/cmd/morphic/edgecases_test.go index 09a3be6..6b827b1 100644 --- a/cmd/morphic/edgecases_test.go +++ b/cmd/morphic/edgecases_test.go @@ -83,7 +83,7 @@ func TestMain_ExitCode(t *testing.T) { var got int osExit = func(code int) { got = code } - os.Args = []string{"morphic"} // no subcommand → usage → exit 2 + os.Args = []string{"morphic", "bogus"} // unknown command → usage → exit 2 main() @@ -121,38 +121,6 @@ func TestRunParse_NilDocumentReturnsOne(t *testing.T) { assert.Contains(t, stderr.String(), "openapi/unsupported-version") } -func TestRunParse_UnknownFlagIsUsageError(t *testing.T) { - t.Parallel() - spec := writeFile(t, "spec.yaml", testspec.Tiny) - var stdout, stderr bytes.Buffer - - code := run([]string{"compile", spec, "--bogus"}, &stdout, &stderr) - - assert.Equal(t, 2, code) - assert.Contains(t, stderr.String(), "usage") -} - -func TestRunParse_WrongPositionalCount(t *testing.T) { - t.Parallel() - spec := writeFile(t, "spec.yaml", testspec.Tiny) - tests := []struct { - name string - args []string - }{ - {"no spec file", []string{"compile"}}, - {"two spec files", []string{"compile", spec, spec}}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - var stdout, stderr bytes.Buffer - code := run(tt.args, &stdout, &stderr) - assert.Equal(t, 2, code) - assert.Contains(t, stderr.String(), "requires exactly one spec file") - }) - } -} - func TestRunParse_SkipValidateToStdout(t *testing.T) { t.Parallel() spec := writeFile(t, "spec.yaml", testspec.Tiny) diff --git a/cmd/morphic/help.go b/cmd/morphic/help.go new file mode 100644 index 0000000..3b0175a --- /dev/null +++ b/cmd/morphic/help.go @@ -0,0 +1,92 @@ +package main + +import ( + "fmt" + "io" +) + +// rootDescription is the one-paragraph summary shown in root help. +const rootDescription = "morphic lowers an API spec into Morphic IR." + +// isHelpFlag reports whether arg asks for help rather than naming work to do. +func isHelpFlag(arg string) bool { + return arg == "-h" || arg == "--help" || arg == "-help" +} + +// writeRootHelp writes the top-level help text to w: the synopsis, what morphic +// is, the command list built from the command table, and how to go deeper. +func writeRootHelp(w io.Writer) { + emitf(w, "usage:\n morphic [flags]\n\n%s\n\ncommands:\n", rootDescription) + for _, c := range commands() { + emitf(w, " %-9s %s\n", c.name, c.summary) + } + emitf(w, "\nrun \"morphic help \" for command details.\n") +} + +// writeCommandHelp writes c's full help text to w: synopsis, description, and +// c's own flag table. +func writeCommandHelp(w io.Writer, c command) { + emitf(w, "usage:\n %s\n\n%s\n\nflags:\n", c.usage, c.description) + c.printFlags(w) +} + +// rootUsageError reports a misuse of morphic itself: one reason line, the root +// help, exit 2. It never touches stdout. +// +// reason is a finished string, not a format — see compileUsageError for why. +func rootUsageError(stderr io.Writer, reason string) int { + emitf(stderr, "morphic: %s\n", reason) + writeRootHelp(stderr) + return 2 +} + +// writeCommandUsage writes the short pointer shown after a misuse of c: the +// synopsis and where the details live, never the whole flag table. +func writeCommandUsage(w io.Writer, c command) { + emitf(w, "usage:\n %s\nrun \"morphic help %s\" for details.\n", c.usage, c.name) +} + +// filterHelpTokens returns args with every help-flag token removed. help +// takes only a bare positional command name and defines no flags of its own, +// so a help-flag token can never be a legitimate value for it — stripping +// these tokens first lets the argument-count and lookup logic in runHelp run +// on whatever command name, if any, remains. This filtering approach is safe +// here specifically because help has no flags; runCompile must keep detecting +// help via errors.Is(err, flag.ErrHelp) instead of pre-scanning argv. +func filterHelpTokens(args []string) []string { + names := make([]string, 0, len(args)) + for _, arg := range args { + if isHelpFlag(arg) { + continue + } + names = append(names, arg) + } + return names +} + +// runHelp implements every root-level request for help, whether spelled as the +// `help` subcommand or as a leading help flag: no command name prints root +// help, one name prints that command's help, and anything else is misuse. +// Help-flag tokens (`-h`, `--help`, `-help`) are stripped from args before the +// argument-count check runs, so `help compile --help` prints compile help +// rather than being conflated with `help --help`, and `help bogus --help` is +// still rejected as misuse rather than silently returning root help — a help +// flag no longer masks a mistyped or extra command name. +func runHelp(args []string, stdout, stderr io.Writer) int { + names := filterHelpTokens(args) + if len(names) == 0 { + writeRootHelp(stdout) + return 0 + } + if len(names) > 1 { + return rootUsageError(stderr, "help accepts at most one command") + } + + c, ok := lookup(names[0]) + if !ok { + return rootUsageError(stderr, fmt.Sprintf("unknown command %q", names[0])) + } + + writeCommandHelp(stdout, c) + return 0 +} diff --git a/cmd/morphic/help_test.go b/cmd/morphic/help_test.go new file mode 100644 index 0000000..f421ac9 --- /dev/null +++ b/cmd/morphic/help_test.go @@ -0,0 +1,133 @@ +package main + +import ( + "bytes" + "os" + "path/filepath" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/internal/testspec" +) + +func TestRun_HelpForms(t *testing.T) { + t.Parallel() + spec := writeFile(t, "spec.yaml", testspec.Tiny) + + tests := []struct { + name string + args []string + }{ + {"no arguments", nil}, + {"root -h", []string{"-h"}}, + {"root --help", []string{"--help"}}, + {"root -help", []string{"-help"}}, + {"root help word", []string{"help"}}, + {"help compile", []string{"help", "compile"}}, + {"help -h", []string{"help", "-h"}}, + {"help --help", []string{"help", "--help"}}, + {"help compile --help", []string{"help", "compile", "--help"}}, + {"help compile -h", []string{"help", "compile", "-h"}}, + {"root -h compile", []string{"-h", "compile"}}, + {"root --help compile", []string{"--help", "compile"}}, + {"compile -h", []string{"compile", "-h"}}, + {"compile --help", []string{"compile", "--help"}}, + {"compile spec --help", []string{"compile", spec, "--help"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + var stdout, stderr bytes.Buffer + + assert.Equal(t, 0, run(tt.args, &stdout, &stderr)) + assert.Contains(t, stdout.String(), "usage:") + assert.Empty(t, stderr.String(), "help must never write to stderr") + }) + } +} + +// TestRun_HelpFlagAsFlagValue pins the property the help design relies on: +// help is detected via errors.Is(err, flag.ErrHelp) from flag.Parse, never by +// pre-scanning argv for "--help". So "-o --help" must consume "--help" as the +// value of -o and compile normally, not print help. This guards against a +// future refactor of runCompile that pre-scans args and would keep full +// statement coverage while silently breaking that distinction. Not run in +// parallel: it changes the process working directory so -o's relative value +// resolves to a file literally named "--help". +func TestRun_HelpFlagAsFlagValue(t *testing.T) { + dir := t.TempDir() + prevWD, err := os.Getwd() + require.NoError(t, err) + require.NoError(t, os.Chdir(dir)) + t.Cleanup(func() { require.NoError(t, os.Chdir(prevWD)) }) + + spec := writeFile(t, "spec.yaml", testspec.Tiny) + var stdout, stderr bytes.Buffer + + code := run([]string{"compile", "-o", "--help", spec}, &stdout, &stderr) + + require.Equal(t, 0, code, "stderr: %s", stderr.String()) + assert.NotContains(t, stdout.String(), "usage:", + "--help must be consumed as -o's value, not treated as a help request") + raw, err := os.ReadFile(filepath.Join(dir, "--help")) + require.NoError(t, err) + assert.Contains(t, string(raw), `"name": "Tiny"`) +} + +func TestRun_HelpFormsAgree(t *testing.T) { + t.Parallel() + + forms := [][]string{ + {"compile", "--help"}, + {"compile", "-h"}, + {"help", "compile"}, + {"help", "compile", "--help"}, + {"help", "compile", "-h"}, + {"-h", "compile"}, + {"--help", "compile"}, + } + + var want string + for i, args := range forms { + var stdout, stderr bytes.Buffer + require.Equal(t, 0, run(args, &stdout, &stderr), "stderr: %s", stderr.String()) + if i == 0 { + want = stdout.String() + require.NotEmpty(t, want) + continue + } + assert.Empty(t, cmp.Diff(want, stdout.String()), "help text differs for %v", args) + } +} + +func TestRun_CompileHelpListsEveryFlag(t *testing.T) { + t.Parallel() + + var stdout, stderr bytes.Buffer + require.Equal(t, 0, run([]string{"help", "compile"}, &stdout, &stderr)) + + // Read the flag names back out of the help text rather than asking whether + // it contains each name: a one-letter name like "o" is a substring of almost + // any text, so a containment check passes for help that documents nothing. + assert.ElementsMatch(t, compileFlagNames, flagNamesIn(stdout.String()), + "compile help must document exactly the flags compile accepts") +} + +func TestRootHelp_ListsEveryCommand(t *testing.T) { + t.Parallel() + + var stdout, stderr bytes.Buffer + require.Equal(t, 0, run(nil, &stdout, &stderr)) + + got := stdout.String() + table := commands() + require.NotEmpty(t, table, "the command table must not be empty") + for _, c := range table { + assert.Contains(t, got, c.name) + assert.Contains(t, got, c.summary) + } +} diff --git a/cmd/morphic/main.go b/cmd/morphic/main.go index 3f25662..056e52a 100644 --- a/cmd/morphic/main.go +++ b/cmd/morphic/main.go @@ -23,14 +23,20 @@ func main() { // tests can drive the CLI without a subprocess; only main calls os.Exit. func run(args []string, stdout, stderr io.Writer) int { if len(args) == 0 { - printUsage(stderr) - return 2 + writeRootHelp(stdout) + return 0 } + // A leading help flag is the help command spelled differently, so it takes + // the same path rather than a shortcut to root help. That is what makes + // "morphic -h compile" print compile's help instead of silently dropping + // the name, and "morphic -h bogus" report it instead of masking it. + if args[0] == "help" || isHelpFlag(args[0]) { + return runHelp(args[1:], stdout, stderr) + } + c, ok := lookup(args[0]) if !ok { - emitf(stderr, "morphic: unknown command %q\n", args[0]) - printUsage(stderr) - return 2 + return rootUsageError(stderr, fmt.Sprintf("unknown command %q", args[0])) } return c.run(args[1:], stdout, stderr) } @@ -40,18 +46,3 @@ func run(args []string, stdout, stderr io.Writer) int { func emitf(w io.Writer, format string, args ...any) { _, _ = fmt.Fprintf(w, format, args...) } - -// printUsage writes the usage text to w. -func printUsage(w io.Writer) { - emitf(w, "%s\n", usage) -} - -const usage = `usage: - morphic compile [-o out.json] [--fail-on error|warning] [--skip-validate] - [--explain ] - -compile lowers an API spec (OpenAPI 3.x) into Morphic IR JSON. - ---explain reports what compiling produced at one source coordinate — the type -node interned there, the coordinates interned beneath it, and the diagnostics -stamped at it — instead of writing the document.`