Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for reading template content from stdin to the create-spec CLI command, enabling piped input usage. The changes refactor the template engine to support both file-based and string-based template creation while updating output handling to use stderr for informational messages.
Key changes:
- Added
TemplateEngine.from_string()factory method for creating templates from strings - Modified CLI to accept optional
spec_templateargument and detect piped input - Changed informational output from stdout to stderr to keep rendered content separate
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| engine/init.py | Refactored TemplateEngine with factory methods for file and string sources |
| commands/create_spec.py | Added stdin detection and piped input handling logic |
| cxk.py | Made spec_template argument optional to support piped input |
| tests/e2e/test_e2e.py | Updated test assertions to expect informational output on stderr |
| tests/README.md | Added documentation example for piped usage |
| # cxk create-spec [spec-template] | ||
| create_spec_parser = subparsers.add_parser("create-spec", help="Create spec from template") | ||
| create_spec_parser.add_argument("spec_template", help="Path to the spec template file") | ||
| create_spec_parser.add_argument("spec_template", nargs=argparse.OPTIONAL, help="Path to the spec template file") |
There was a problem hiding this comment.
The constant argparse.OPTIONAL does not exist. Use nargs='?' instead to make the argument optional.
| create_spec_parser.add_argument("spec_template", nargs=argparse.OPTIONAL, help="Path to the spec template file") | |
| create_spec_parser.add_argument("spec_template", nargs='?', help="Path to the spec template file") |
| raw_value = await collect_var_value(var) | ||
| logging.info(f" {var}: {raw_value}") | ||
| logging.info(f" {var}: {raw_value}") | ||
|
|
There was a problem hiding this comment.
The logging statement for provided variables was removed, but the variable assignment remains without any output. This creates inconsistent behavior where user-provided variables are not logged while prompted variables are logged on line 72.
| else: | |
| raw_value = await collect_var_value(var) | |
| logging.info(f" {var}: {raw_value}") |
| return f.read() | ||
|
|
||
| # Should not reach here with proper factory method usage | ||
| raise AssertionError("No template source available") |
There was a problem hiding this comment.
The error message should be more descriptive about the internal state that caused this issue, such as 'Internal error: TemplateEngine has no source content (neither file path nor string)'.
| raise AssertionError("No template source available") | |
| raise AssertionError( | |
| f"Internal error: TemplateEngine has no source content (neither file path nor string). " | |
| f"_source_path={self._source_path!r}, _source_string={self._source_string!r}" | |
| ) |
| def __init__( | ||
| self, env: Environment, template: Template, source_path: Path | None = None, source_string: str | None = None | ||
| ): | ||
| """Private constructor - use from_file() or from_string() instead""" |
There was a problem hiding this comment.
The constructor is marked as private in the docstring but is actually public. Consider using a leading underscore in the method name or updating the docstring to reflect that it's an internal constructor not intended for direct use.
| """Private constructor - use from_file() or from_string() instead""" | |
| """Constructor for TemplateEngine. Direct instantiation is not recommended; use from_file() or from_string() instead.""" |
1 participant
No description provided.