-
Notifications
You must be signed in to change notification settings - Fork 22
feat(emitter):add structured data output with JSON and table formatting #380
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(emitter):add structured data output with JSON and table formatting #380
Conversation
| class BaseFormatter(ABC): | ||
| @abstractmethod | ||
| def format(self,data:TabularData,headers:dict[str,str] | None=None)->str: | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running make format should fix the formatting issue here
lengau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, please fix the broken tests and lint errors here.
f33ec01 to
7a4f4b5
Compare
ae4958a to
00bd742
Compare
craft_cli/pytest_plugin.py
Outdated
| # messages.emit._initiated = False | ||
| # messages.emit._stopped = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be here?
examples.py
Outdated
| # emit.message("Table output:") | ||
| # emit.data(sample_data,format="table") | ||
|
|
||
| # emit.structured(sample_data,fmt=fmt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
Problem solved, and no doc changes that require my review.
pyproject.toml
Outdated
| "FBT002", # Boolean default value in function definition (preserving for backwards-comp) | ||
| "S101", # Use of `assert` detected | ||
| ] | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something looks wrong with this file. Lot's of unnecessary changes
2d05361 to
e64a8dc
Compare
e64a8dc to
2afdf38
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds structured data output capabilities to the Emitter class, supporting JSON and table formatting for machine and human-readable output. The implementation includes a formatter registry pattern, CLI integration, and comprehensive test coverage.
Key changes:
- Added
BaseFormatter,JSONFormatter, andTableFormatterclasses for flexible output formatting - Implemented
Emitter.data(),Emitter.table(), andEmitter.json()methods for structured data output - Added
--formatCLI argument with json/table choices (filtered from help output)
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| craft_cli/messages.py | Core formatter classes and Emitter methods for structured data output |
| craft_cli/dispatcher.py | Added --format CLI argument and filtered it from command help |
| craft_cli/helptexts.py | Fixed help text generation when options list is empty |
| craft_cli/printer.py | Added raw_output parameter support to show() method |
| examples.py | Added example_36 demonstrating structured output formats |
| tests/unit/test_messages_emitter.py | Added unit tests for formatters and updated double init test |
| tests/unit/test_help.py | Updated tests to reflect filtered global options from help |
| tests/integration/test_messages_integration.py | Adjusted timing thresholds for test stability |
| craft_cli/init.py | Exposed Emitter and complete in public API |
| craft_cli/pytest_plugin.py | Added type ignore comments for pytest imports |
| craft_cli/completion/completion.py | Added explicit type annotation for template render |
| docs/conf.py | Added type ignore comment for sphinx import |
| pyproject.toml | File deleted (full file removal) |
Comments suppressed due to low confidence (1)
pyproject.toml:1
- The entire pyproject.toml file has been deleted. This removes critical project metadata, dependencies, and build configuration. This change should be reverted unless there's a deliberate migration to a different build system.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
| formatter = self._formatters.get(output_format) | ||
| if not formatter: | ||
| raise ValueError(f"Unsupported format: {format}") |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message references undefined variable format instead of the parameter output_format. This will raise a NameError.
| raise ValueError(f"Unsupported format: {format}") | |
| raise ValueError(f"Unsupported format: {output_format}") |
| _ = headers | ||
| """Format a list of rows into a table string.""" |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring is placed after the parameter line and unused variable assignment. Move this docstring to line 84, immediately after the method signature.
| _ = headers | |
| """Format a list of rows into a table string.""" | |
| """Format a list of rows into a table string.""" | |
| _ = headers |
|
|
||
| def example_36(*args: str) -> None: | ||
| """Demonstrate structured output formats.""" | ||
| print(">>> Running example_36 with args:", args) |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message incorrectly refers to 'example_33' instead of 'example_36', as shown in the description. This appears to be a copy-paste error.
| Example: | ||
| ------- | ||
| >>> formatter = JsonFormatter() |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Class name in docstring example should be JSONFormatter (matching the actual class name) instead of JsonFormatter.
| >>> formatter = JsonFormatter() | |
| >>> formatter = JSONFormatter() |
| Example: | ||
| ------- | ||
| >>> formatter = JsonFormatter() | ||
| >>> formatter.format([{"name":"Alice","age":30}) |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example code is missing a closing bracket ] and has unbalanced parentheses. Should be formatter.format([{\"name\":\"Alice\",\"age\":30}]).
| >>> formatter.format([{"name":"Alice","age":30}) | |
| >>> formatter.format([{"name":"Alice","age":30}]) |
| messages, "_get_log_filepath", lambda appname: tmp_path / FAKE_LOG_NAME | ||
| ) | ||
|
|
||
| monkeypatch.setattr(messages, "_get_log_filepath", None) |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting _get_log_filepath to None will cause an error when it's called as a function. This should be a lambda or mock function that returns a path.
|
|
||
| class _CustomArgumentParser(argparse.ArgumentParser): | ||
| """ArgumentParser with custom error manager.""" | ||
|
|
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This class attribute appears to be added without context. If this is documenting an existing attribute for type checking, consider adding a comment explaining its purpose.
| # Type annotation for instance attribute set in __init__; aids type checking. |
Fixes : #348
make lint && make test?This PR extends the
Emitterclass to support structured data output in multiple formats(JSON,table), with a registry for adding formats.Implementation Details
stdoutTableFormatterfor human-readable tables,JSONFormatterfor machine-readable JSONEmitter.data()method which accepts structured data(list[dict]) and uses registered formatter based on the--formatflag.JSONFormatterto fetch JSON outputs withjson.dumpsTableFormatterfor printing rows and columnsEmitter.register_formate(name,formatter)allows registering new formatsjson,table}example_36in examples.py to demo usage.test_messages_emitter.pyTableFomattter, handles list ofdicts, empty list anddictinputJSONFormatterserializes list of dataExample Run