Script refactor #696
Script refactor #696
Conversation
…s whether a command returned stop. This means the command loop will stop once transcript generation completes if a command did return stop. Otherwise there is a risk in continuing to run if the application's state has already been marked to close.
… stop value. The command loop will also terminate.
…oolean constant and was only used internally
…lication to quit Implemented an antediluvian TODO
… order to present a better error message to user
…o return whether a command returned True for stop. Added stop to CommandResult so pyscripts can now know the return value of a command's do_* function.
Codecov Report
@@ Coverage Diff @@
## master #696 +/- ##
==========================================
+ Coverage 95.46% 95.47% +<.01%
==========================================
Files 11 11
Lines 3241 3248 +7
==========================================
+ Hits 3094 3101 +7
Misses 147 147
Continue to review full report at Codecov.
|
tleonhardt
left a comment
tleonhardt
left a comment
There was a problem hiding this comment.
Overall looks good, but I'd recommend changing how we deal with running commands from the cmdqueue in the main loop so that it is safe for multi-threaded applications.
| self.poutput('{}{}'.format(self.prompt, line)) | ||
|
|
||
| Example: cmd_obj.runcmds_plus_hooks(['load myscript.txt']) | ||
| if self.onecmd_plus_hooks(line): |
There was a problem hiding this comment.
Ok, so now we just run all of the commands in a loop instead of appending them to the cmdqueue. That seems reasonable. I'm fearful that there could be some really obscure edge case where this would behave very differently, but I can't think of any right now.
|
Uh oh, I'm getting some push back here on 2 issues. Here is my thinking on both. Command Queue IssueI agree the code to run commands in The problem is that cmd2 has at least 6 places that could be considered a command loop.
Only For instance, suppose I have a command called save that adds a command called print_results to the queue. Then at the command line, the user runs these commands: My history would be: If I then do,
Transcript Generation IssueThis is a tricky one. The behavior before this PR was wrong in that the value of My PR added code to stop transcript generation when any command returned True. That value gets passed up to the command loop, which then stops. Technically the whole application doesn't stop. Just the command loop does. Since transcript generation is actually running commands, then behavior should be identical to running these same commands from the CLI or scripts. I agree that it seems weird to terminate the command loop after transcript generation, but it also seems weird to put quit in a transcript. What I'm trying to do is have consistent behavior across all ways of running commands. Basically, if a command tells the command loop to stop, then it should in all cases, regardless of how it was run. I wrote this in issue #688. Perhaps I didn't explain well enough.
|
|
@kotfu @tleonhardt
|
allow_cli_args is now an argument to __init__ instead of a cmd2 class member
|
The current unit test failures are pretty weird and I don't understand them, but I have been able to replicate them locally. If I just run However, if I run @kotfu Do these unit test failures make sense to you? |
This is so unit tests pass
|
I made a partial reversion of the last commit to get the unit tests passing again until such as time as we can figure out WTF was going wrong. |
| self.broken_pipe_warning = '' | ||
|
|
||
| # Commands that will run at the beginning of the command loop | ||
| self._startup_commands = [] |
There was a problem hiding this comment.
Changing to the use of the self._startup_commands list instead of using self.cmdqueue inherited from cmd solves the "Command queue issue" alluded to in the comments.
Overall, I think we should do a better job in the Sphinx docs of calling out the elements in cmd which are not used or supported in cmd2. We have a "Note" for this in our Overview. But I think it is long overdue that we expand upon that.
…it__()" This reverts commit f42cdb2.
…ne of the commands run while generating a transcript
…returns True for stop
|
@kotfu If you get a chance to look at this, there are a lot of changes. I think @kmvanbrunt addressed all of the concerns we both had, but I'd appreciate any further feedback you might have. The major fundamental change is that I can't think of any negative unintended consequences. But this is a major change in implementation, so I'd appreciate another brain thinking about it. |
|
@teto Can you verify that this PR is getting you most of what you want in terms of the transcript testing and generation changes you were looking for? |
cmdqueue. This allows easy capture of the entire script's output.CommandResultcalledstopwhich is the return value ofonecmd_plus_hooksafter it runs the given command line.eoscommand that was used to keep track of when a text script's commands endedcmd2member called_STOP_AND_EXITsince it was just a boolean value that should always be Truecmd2member called_should_quitsincePyscriptBridgenow handles this logiccmd.cmdqueue_cmdloop()suppressed exceptions by returning from within itsfinallycodeallow_cli_argsis now an argument to init instead of acmd2class memberFixes #687
Fixes #688
Fixes #692