Implement CLI output formatting enhancements#47
Merged
Conversation
- Define formatting.py API: emit_json, emit_text, emit_error, emit_table, emit_list, format_key_value, progress_status - Document migration plan for replacing duplicated _write/_emit_error helpers - Address all acceptance criteria from issue #33
- Create cli/formatting.py with reusable helpers: emit_json, emit_text, emit_error, emit_table, emit_list, format_key_value, progress_status - Migrate all 9 CLI modules to use centralized formatters, removing duplicated _write() and _error() helpers - Add emit_table with column-aligned headers to referrer list output - Use format_key_value for manifest info aligned key-value display - Replace inline json.dumps/click.echo patterns with emit_json - Replace stderr progress messages with progress_status in layout push - Standardize error format to 'Error [ref]: reason' across all commands - Fix dead code in ping command for non-reachable result handling - Add 19 unit tests for all formatting helpers - Update test_manifest_cli for new key-value alignment format
Contributor
There was a problem hiding this comment.
Pull request overview
Centralizes common CLI output logic (JSON/text/file writes, errors, tables) into a new regshape.cli.formatting module and migrates multiple CLI commands to use it for more consistent output.
Changes:
- Added
src/regshape/cli/formatting.pywith reusable emit/format helpers (+ unit tests). - Migrated CLI modules (manifest/tag/catalog/referrer/blob/auth/docker/layout/ping) away from duplicated
_write()/_error()implementations. - Updated
manifest infooutput formatting and adjusted tests accordingly.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/regshape/tests/test_manifest_cli.py | Adjusts assertions to match updated manifest info formatting. |
| src/regshape/tests/test_formatting.py | Adds unit tests for new CLI formatting helpers. |
| src/regshape/cli/formatting.py | Introduces centralized output helpers (JSON/text/errors/tables/lists/key-values). |
| src/regshape/cli/manifest.py | Switches manifest commands to formatting helpers; formats info via aligned key/value output. |
| src/regshape/cli/tag.py | Replaces ad-hoc output/error handling with formatting helpers. |
| src/regshape/cli/catalog.py | Replaces duplicated write/error helpers with centralized emitters. |
| src/regshape/cli/referrer.py | Uses formatting helpers; adds table output for terminal display. |
| src/regshape/cli/blob.py | Uses centralized error/JSON emission helpers. |
| src/regshape/cli/auth.py | Uses centralized error emission (and exits via helper). |
| src/regshape/cli/docker.py | Uses centralized error/JSON emission helpers. |
| src/regshape/cli/layout.py | Uses centralized error/JSON emission; routes progress/status text via progress_status. |
| src/regshape/cli/ping.py | Uses centralized JSON emission; refactors error formatting paths. |
| specs/cli/formatting.md | Adds design/spec documentation for the new formatting module and migration plan. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
progress_status is imported but never used in this module, which will trip basic linting/static checks and adds noise. Either remove the import or use it for any intended status output. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Toddy Mladenov <me@toddysm.com>
The spec states that when headers are provided to emit_table, they are printed “with a separator line”, but the implementation in src/regshape/cli/formatting.py prints only the header row (no separator). Either update the spec or add the separator line behavior so the documentation matches reality. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Toddy Mladenov <me@toddysm.com>
The early return when result.manifests is empty means --output no longer creates/writes the requested file (and stdout output changes from a blank line to no output). If callers rely on the output file being created even when there are zero referrers, consider emitting an empty string via emit_text("", output) when output is set (and optionally emitting nothing for stdout).
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Toddy Mladenov <me@toddysm.com>
TestEmitTable.test_column_alignment computes col2_positions but never asserts anything about it, so it doesn't actually validate alignment (it only asserts the line count). This makes the test a false positive and won't catch regressions in emit_table spacing. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Toddy Mladenov <me@toddysm.com>
Unused imports (os, tempfile, patch) in this test module add noise and may fail linting if/when it’s enabled. Please remove them or use them. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Toddy Mladenov <me@toddysm.com>
Docs say format_key_value(...) returns output with “right-aligned keys”, but the implemented behavior left-pads keys with ljust() so the separator is aligned (keys are left-aligned). Update the wording/example to match the actual alignment to avoid confusing future contributors. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Toddy Mladenov <me@toddysm.com>
- Add err: bool parameter to emit_text() and emit_json() so callers can direct output to stderr, keeping stdout machine-parseable - Update ping _error() to pass err=True for JSON error output - Add tests for stderr routing in both emit_json and emit_text
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the CLI output formatting enhancements specified in #33. Creates a centralized
cli/formatting.pymodule with reusable output helpers and migrates all 9 CLI modules to use them, eliminating duplicated code and improving output consistency.Closes #33
Changes
New module:
src/regshape/cli/formatting.pySeven stateless helper functions that replace scattered, duplicated patterns:
emit_json(data, output_path)emit_text(content, output_path)_write())emit_error(reference, reason, exit_code)Error [ref]: reasonto stderr + exitemit_table(rows, headers)emit_list(items, output_path)format_key_value(pairs)progress_status(message)Migrated CLI modules (9 files)
_write/_error→emit_text/emit_json/emit_error/format_key_value_write/_error→emit_list/emit_json/emit_error_write/_error→emit_list/emit_json/emit_error_write/_error→emit_table/emit_json/emit_error; referrer list now shows column-aligned headers (DIGEST,ARTIFACT TYPE,SIZE)_error/json.dumps→emit_error/emit_jsonjson.dumps→emit_json; fixed dead code for non-reachable result path_error→emit_error; standardized from"Error for"to"Error [...]"format_error/json.dumps/click.echo(err=True)→emit_error/emit_json/progress_status_error/json.dumps→emit_error/emit_jsonTests
test_formatting.py— 19 new tests covering all 7 helperstest_manifest_cli.py— Updated assertion to match newformat_key_valuealignmentDesign spec
specs/cli/formatting.md— Full design specification (committed in prior push)What was removed
_write()helper definitions (manifest, tag, catalog, referrer)_error()helper definitions with minor format variationsjson.dumps(..., indent=2)+click.echo()patterns throughoutsys.exit(1)Testing
All 19 new formatting tests pass. Full suite: 1036 passed, 5 failed (all 5 pre-existing in
test_catalog_operationsandtest_transport_client, unrelated to this PR).