Conversation
ce945dd to
9af6bf8
Compare
9af6bf8 to
1162799
Compare
TomNaessens
left a comment
There was a problem hiding this comment.
Nice! Had a look, mostly from a sanity check perspective and didn't spot anything weird or out of place.
|
I know it's already merged, but to be sure we didn't miss any obvious things I requested a copilot review just now. |
There was a problem hiding this comment.
Pull request overview
This PR implements file handling for stdin with support for file redirects and dynamically generated files, addressing issue #600. The implementation allows test suites to specify stdin using file paths (e.g., stdin: {path: "hello.txt"}), with support for:
- Simple file references where content is read from a file
- Dynamically generated files where content is specified inline or copied from another file to a different name
- Display of file redirects in test descriptions (e.g.,
$ submission < hello.txt)
The PR also refactors the parsing decorators (@fallback_field and @ignore_field) to support decorator stacking via a new _chain_structure_hook mechanism, ensuring top-to-bottom execution order.
Changes:
- Core data model updated:
TextData.datarenamed toTextData.content, addedTextData.pathfield, introducedContentPathclass - Parsing decorators refactored to support chaining without requiring converter parameter
- Execution logic enhanced to create dynamically generated files at runtime
- DSL parser extended to handle
!pathYAML tag and file-based stdin definitions - Comprehensive test coverage added for legacy field support, decorator chaining, and stdin file handling
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tested/testsuite.py | Renamed data to content, added ContentPath class, added path field and is_dynamically_generated() method to TextData |
| tested/parsing.py | Refactored decorators to use _chain_structure_hook for composable hooks, removed converter parameter requirement |
| tested/judge/planning.py | Added DynamicallyGeneratedFile class and get_dynamically_generated_files() method |
| tested/judge/execution.py | Implemented file creation logic for dynamically generated files |
| tested/languages/generation.py | Added stdin file redirect display support ($ submission < file.txt) |
| tested/languages/preparation.py | Updated to use content instead of data |
| tested/dsl/translate_parser.py | Added parsing logic for stdin file definitions, !path tag support, ContentPathString class |
| tested/dsl/schema.json | Added fileData definition for stdin, updated to allow path or content fields |
| tested/dsl/schema-strict.json | Extended fileData to support path type in content field |
| tested/configs.py | Removed converter parameter from @fallback_field |
| tested/oracles/common.py | Removed converter parameter from @fallback_field |
| tests/tested-draft7.json | Added "path" to simpleTypes enum for custom YAML tag support |
| tests/test_testsuite_legacy.py | New tests for legacy field support and decorator ordering |
| tests/test_parsing.py | New comprehensive tests for parsing decorators |
| tests/test_dsl_legacy.py | New tests for legacy DSL field support |
| tests/test_suite.py | Updated assertions to use content instead of data |
| tests/test_oracles_builtin.py | Updated to use content instead of data |
| tests/test_functionality.py | Updated to use content instead of data |
| tests/test_dsl_yaml.py | Updated to use content, added stdin file handling tests |
| tests/test_io_exercises.py | Added tests for stdin file handling |
| tests/exercises/echo/evaluation/plan.yaml | New test plan with stdin file examples |
| tests/exercises/echo/evaluation/plan-dynamic.yaml | New test plan with dynamically generated file examples |
| tests/exercises/echo/evaluation/input.txt | Input file for testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # If we have both stdin and arguments, we use a here-document. | ||
| if case.input.arguments and stdin: | ||
| if stdin and isinstance(stdin, Path): | ||
| text = f"${args} < {stdin}" |
There was a problem hiding this comment.
The format string should be f"$ {args} < {stdin}" not f"${args} < {stdin}". The dollar sign in shell prompts is conventionally followed by a space before the command. This inconsistency with line 128 (args = f"$ {command}") makes the output look incorrect.
| text = f"${args} < {stdin}" | |
| text = f"$ {args} < {stdin}" |
| if isinstance(dynamically_generated_file.content, ContentPath): | ||
| _logger.debug( | ||
| f"Copying input file %s to %s", | ||
| dynamically_generated_file.content.path, | ||
| destination, | ||
| ) | ||
| source_file = ( | ||
| bundle.config.resources / dynamically_generated_file.content.path | ||
| ) | ||
| shutil.copy2(source_file, destination) | ||
| else: | ||
| _logger.debug(f"Creating dynamically generated file %s", destination) | ||
| destination.parent.mkdir(parents=True, exist_ok=True) | ||
| destination.write_text(dynamically_generated_file.content) |
There was a problem hiding this comment.
The destination.parent.mkdir(parents=True, exist_ok=True) call should also be made before copying the file in the ContentPath branch (line 199). If the destination path includes subdirectories (e.g., "subdir/input.txt"), the copy will fail because the parent directory doesn't exist. Move this mkdir call before the if-else block or add it to both branches.
| assert ( | ||
| False | ||
| ), f"Invalid stdin content is required but got {type(raw_content)}" |
There was a problem hiding this comment.
The error message "Invalid stdin content is required but got..." is misleading when neither content nor path is provided. Consider improving it to something like "stdin requires either 'content' or 'path' field, but neither was provided" to better reflect the actual error condition.
| assert ( | |
| False | |
| ), f"Invalid stdin content is required but got {type(raw_content)}" | |
| if raw_content is None and raw_path is None: | |
| assert False, ( | |
| "stdin requires either 'content' or 'path' field, " | |
| "but neither was provided" | |
| ) | |
| else: | |
| assert ( | |
| False | |
| ), ( | |
| "Invalid stdin configuration: expected 'content' as a string " | |
| f"or 'path' as a string, but got content type {type(raw_content)} " | |
| f"and path type {type(raw_path)}" | |
| ) |
Revert "Merge pull request #608 from dodona-edu/enhancement/stdin"
This PR implements the
stdinpart of #600 , based on the ideas from #577.For example:
will now result in:
{"command": "start-judgement"} {"title": "stdin", "command": "start-tab"} {"command": "start-context"} {"description": {"description": "$ submission < hello.txt", "format": "console"}, "command": "start-testcase"} {"expected": "hello world!\n", "channel": "stdout", "command": "start-test"} {"generated": "hello world!\n", "status": {"enum": "correct"}, "command": "close-test"} {"command": "close-testcase"} {"data": {"files": [{"path": "hello.txt"}]}, "command": "close-context"} {"command": "close-tab"} {"command": "close-judgement"}Technical
stdinstdinfiledefinitions (usingcontentandpath)cattrsdecorators, and adds a bunch of tests for that