Pyscript updates #405
Pyscript updates #405
Conversation
…t. Added pyscript bridge to ipy command. Saving progress.
…ation command from pyscript. Added support for tab completing application commands in ipython shell Updated unit tests scripts to set cmd_echo to True to validate command output.
Codecov Report
@@ Coverage Diff @@
## master #405 +/- ##
==========================================
- Coverage 90.15% 89.76% -0.39%
==========================================
Files 8 8
Lines 2528 2560 +32
==========================================
+ Hits 2279 2298 +19
- Misses 249 262 +13
Continue to review full report at Codecov.
|
tleonhardt
left a comment
tleonhardt
left a comment
There was a problem hiding this comment.
Consider my comments, particularly regarding a default value for echo
| banner = 'Entering an embedded IPython shell type quit() or <Ctrl>-d to exit ...' | ||
| exit_msg = 'Leaving IPython, back to {}'.format(sys.argv[0]) | ||
| embed(banner1=banner, exit_msg=exit_msg) | ||
| from .pyscript_bridge import PyscriptBridge |
There was a problem hiding this comment.
Thanks for making the features added by pyscript_bridge available consistently using either the py or ipy commands.
cmd2/pyscript_bridge.py
Outdated
| class CopyStream(object): | ||
| """Copies all data written to a stream""" | ||
| def __init__(self, inner_stream): | ||
| def __init__(self, inner_stream, echo): |
There was a problem hiding this comment.
It kinds of feels to me like echo should have a default value and maybe it should be False. I think that would make the behavior as close as possible to being backwards compatible.
What are your thoughts?
NOTE: This comment applies to other functions in this file that also now accept the echo argument.
cmd2/pyscript_bridge.py
Outdated
| class CopyStream(object): | ||
| """Copies all data written to a stream""" | ||
| def __init__(self, inner_stream): | ||
| def __init__(self, inner_stream, echo): |
There was a problem hiding this comment.
Recommend adding optional type hinting for new arguments unless you are planning to backport to the python2 branch.
|
|
||
|
|
||
| def test_pyscript_custom_name(ps_echo, capsys): | ||
| @pytest.mark.parametrize('expected, pyscript_file', [ |
There was a problem hiding this comment.
Thanks for adding some unit tests
tleonhardt
left a comment
tleonhardt
left a comment
There was a problem hiding this comment.
Consider my one additional comment (this time in the correct place)
|
|
||
|
|
||
| def _exec_cmd(cmd2_app, func): | ||
| def _exec_cmd(cmd2_app, func: Callable, echo: bool): |
There was a problem hiding this comment.
Should this echo also default to False?
There was a problem hiding this comment.
Since this is intended to be an internal function I think I'd prefer the caller to be explicit about it.
Applied the data isolation added to the
pycommand to theipycommand.Added stdout/stderr suppression by default when executing application commands from a python script. Setting app.cmd_echo = True enables output.