Executor update - #1
Conversation
| except (OSError, json.JSONDecodeError): | ||
| input_dict = {} |
There was a problem hiding this comment.
Probably better to verbosely return an error instead of silently ignoring invalid input.
| except OSError: | ||
| pass |
There was a problem hiding this comment.
Probably better to verbosely return an error instead of silently treating a file as a code string if it fails to be read as a file. Enforsing that file paths must be pathlib.Path objects makes this easier.
There was a problem hiding this comment.
Properly managed by logging and raising a ValueError
| input: Optional[Dict[str, Any]] = None, | ||
| json_input: Optional[Any] = None, |
There was a problem hiding this comment.
Same comments as for run()
| Returns: | ||
| The executed result when status is ``ok``. | ||
|
|
||
| Raises: |
There was a problem hiding this comment.
On timeout, we should probably raise a executor.TimeoutError, inheriting from executor.ExecuteError and buildins.TimeoutError.
| from pybox import Executor, Config | ||
|
|
||
| cfg = Config( | ||
| timeout=3.0, | ||
| stdout_file=".pybox/stdout.txt", | ||
| stderr_file=".pybox/stderr.txt", | ||
| ) | ||
| executor = Executor(cfg) | ||
| result = executor.run("result = x + y", {"x": 2, "y": 3}) |
There was a problem hiding this comment.
Would it make sence to add >>> prompts to the code examples? That would allow to test them with doctest.
With ipython it is not a problem, but for some people it may create problems for interactively copying the examples into the interpreter. Any thoughts or opinions?
There was a problem hiding this comment.
Hmm, the cleanest solution may be to keep the code blocks without prompt and them test with markdown-pytest?
There was a problem hiding this comment.
I looked into markdown-pytest, but it requires injecting HTML comments above every single code block to register them as tests.
Instead, I've added pytest-codeblocks to our dev dependencies. It automatically picks up standard Python markdown and execute the examples when running pytest --codeblocks README.md
There was a problem hiding this comment.
Update now it skips the unrunnable code blocks, we can discuss the best solution since there are only 3 runnable tests in README
There was a problem hiding this comment.
The result dict also has key: 'duration'
There was a problem hiding this comment.
Updated accordingly and extended README
There was a problem hiding this comment.
I didn't see your changes, let me know if the new version is good
There was a problem hiding this comment.
Isn't it more modern to put the requirements in the pyproject.toml file?
There was a problem hiding this comment.
Removed requirements.txt everything is in pyproject.toml
jesper-friis
left a comment
jesper-friis
left a comment
There was a problem hiding this comment.
Nice work! Here are some suggestions that can be discussed.
Co-authored-by: Jesper Friis <jesper-friis@users.noreply.github.com>
2 participants
Update executor to handle files as input, stdout and stderr files as results.