Fix confusing error message for unsupported output format; remove plain-text as advertised format#11220
Fix confusing error message for unsupported output format; remove plain-text as advertised format#11220
Conversation
…thout plain-text Co-authored-by: brooke-hamilton <45323234+brooke-hamilton@users.noreply.github.com>
|
@copilot fix this. There should not be help text saying that |
…onsistent error messages Co-authored-by: brooke-hamilton <45323234+brooke-hamilton@users.noreply.github.com>
Fixed in a924a54. Added |
|
@copilot does the same fix need to be applied to other commands? |
No, the fix already covers all commands. |
…n, add unit tests, remove unused AllFormats Co-authored-by: brooke-hamilton <45323234+brooke-hamilton@users.noreply.github.com>
Applied all three review comments in ff8d616:
|
Unit Tests4 802 tests +19 4 798 ✅ +19 7m 59s ⏱️ +12s For more details on these failures, see this check. Results for commit 61009ac. ± Comparison against base commit a5b7a93. ♻️ This comment has been updated with latest results. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11220 +/- ##
==========================================
+ Coverage 51.00% 51.12% +0.12%
==========================================
Files 679 679
Lines 43174 43169 -5
==========================================
+ Hits 22019 22072 +53
+ Misses 19035 18964 -71
- Partials 2120 2133 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
… backwards compat - Add NormalizeFormat() in output/formats.go to map plain-text -> table - Add plain-text case in NewFormatter() as defense in depth - Rewrite RequireOutput() to normalize plain-text silently to table - Delete RequireOutputAllowPlainText() and requireOutputFromFormats() - Delete AddOutputFlagWithPlainText() from commonflags - Switch recipe-pack show to use standard AddOutputFlag + RequireOutput - Switch version command to use RequireOutput for consistency - Add tests for NormalizeFormat, NewFormatter(plain-text), and updated RequireOutput behavior - Add table format test case for recipe-pack show Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>
| format, err := cli.RequireOutput(cmd) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if format == "" { | ||
| format = "table" | ||
| } | ||
| r.Format = format |
There was a problem hiding this comment.
The Validate method calls cli.RequireOutput() which reads the "output" flag, but this flag is never registered in the NewCommand function. The version command needs to call commonflags.AddOutputFlag(cmd) in NewCommand() to register the output flag, similar to how other commands (like recipe-pack show) do it. Without this, the command will fail at runtime with a flag not found error.
pkg/cli/clivalidation_test.go
Outdated
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove extra blank lines at the end of the file. Go convention is to have only one blank line at the end of a file.
|
@copilot apply changes based on the comments in this thread |
…n test file Co-authored-by: brooke-hamilton <45323234+brooke-hamilton@users.noreply.github.com>
Applied both changes in 6f65972:
|
Radius functional test overviewClick here to see the test run details
Test Status⌛ Building Radius and pushing container images for functional tests... |
Description
This PR improves the CLI output format handling by:
json,table).plain-textas an advertised output format —plain-textwas only accepted byrad recipe-pack showand produced output identical totable. It is no longer shown in help text or flag descriptions.--output plain-text(e.g. in scripts) will silently get table-formatted output instead of an error.Changes
Output format normalization
NormalizeFormat()inpkg/cli/output/formats.gothat maps"plain-text"→"table"FormatPlainTextcase inNewFormatter()that returnsTableFormatter(defense in depth)Unified validation
RequireOutput()inpkg/cli/clivalidation.goto normalize"plain-text"→"table"before validationRequireOutputAllowPlainText()andrequireOutputFromFormats()— no longer neededAddOutputFlagWithPlainText()frompkg/cli/cmd/commonflags/flags.goCommand updates
rad recipe-pack show: switched fromAddOutputFlagWithPlainText+RequireOutputAllowPlainTextto standardAddOutputFlag+RequireOutputrad version: switched from manual flag reading tocli.RequireOutput()for consistent validation; registered output flag viacommonflags.AddOutputFlag(cmd)inNewCommand()Tests
Test_RequireOutput:"plain-text"now expects"table"(not an error)Test_RequireOutputAllowPlainText(function deleted)pkg/cli/output/formats_test.go— tests forNormalizeFormatpkg/cli/output/formatter_test.go— tests forNewFormatterincluding"plain-text"casepkg/cli/cmd/recipepack/show/show_test.goBackwards Compatibility
rad app list -o jsonrad app list -o tablerad app list -o plain-textrad app list -o textrad app list --helprad recipe-pack show X -o plain-textrad recipe-pack show X -o tablerad recipe-pack show --helpKey point:
"plain-text"was never meaningfully different from"table"— it always produced the same output. This change makes the CLI consistent while ensuring existing scripts continue to work.Type of change
Contributor checklist
Please verify that the PR meets the following requirements, where applicable:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.