Transcript regexes now have predictable, tested, and documented behavior#216
Transcript regexes now have predictable, tested, and documented behavior#216tleonhardt merged 19 commits intopython-cmd2:masterfrom kotfu:fix/transcript_regexes
Conversation
Escapes of slashes don’t work yet.
It output double newlines after each history item
Codecov Report
@@ Coverage Diff @@
## master #216 +/- ##
==========================================
+ Coverage 96.74% 96.81% +0.07%
==========================================
Files 1 1
Lines 1167 1194 +27
==========================================
+ Hits 1129 1156 +27
Misses 38 38
Continue to review full report at Codecov.
|
|
I'll review this significant change later tonight. From a brief look, this looks fantastic! Thanks for the PR. I think you might want to pull the latest stuff from master to pick up some minor changes. |
tleonhardt
left a comment
tleonhardt
left a comment
There was a problem hiding this comment.
Thank you for this fantastic PR.
You did an awesome job. Clear well-commented code, updated straightforward documentation, and thorough unit tests.
|
|
||
| """ | ||
| listformat = '-------------------------[%d]\n%s\n' | ||
| listformat = '-------------------------[{}]\n{}\n' |
There was a problem hiding this comment.
I like it! You are leaving code cleaner than you found it.
I've slowly been converting string formatting to use this newer .format() style with {} instead of the older printf-style formatting, but only when I change some nearby or related code.
| # turn through the loop | ||
| start = second_slash_pos + 1 | ||
| else: | ||
| # No closing slash, we have to add the first slash, |
There was a problem hiding this comment.
Excellent comments throughout. Thanks for taking the time to comment well.
|
|
||
| def _escaped_find(self, regex, s, start, in_regex): | ||
| """ | ||
| Find the next slash in {s} after {start} that is not preceded by a backslash. |
There was a problem hiding this comment.
Just curious, does putting a function argument name inside curly braces within the function docstring do something special? I've got more of a C++ background and am relatively new to Python, so please forgive my ignorance and maybe I will learn something ;-)
| @@ -0,0 +1,161 @@ | |||
| ======================== | |||
There was a problem hiding this comment.
This section is a very nice addition to the existing user manual. It does a great job explaining what transcripts are, how to use them, and what some of the common pitfalls are and how to avoid them. Well done!
| self.stdout.write('\n') | ||
| # self.stdout.write is better than "print", because Cmd can be | ||
| # initialized with a non-standard output destination | ||
| self.poutput(arg) |
There was a problem hiding this comment.
Good catch. I recently converted all of the code within cmd2.py to use .poutput() instead of using stdout.write directly. I didn't go through all of the examples to make sure they were doing the same however.
| do_orate = do_speak # another synonym, but this one takes multi-line input | ||
|
|
||
| @options([ make_option('-r', '--repeat', type="int", help="output [n] times") ]) | ||
| def do_mumble(self, arg, opts=None): |
There was a problem hiding this comment.
I like this addition. It is a convenient way to introduce motivation for using regexes within transcripts due to unpredictable randomness.
|
|
||
| def test_regex_transcript(request, capsys): | ||
| # Create a cmd2.Cmd() instance and make sure basic settings are like we want for test | ||
| @pytest.mark.parametrize('filename, feedback_to_output', [ |
There was a problem hiding this comment.
This was a smart way to expand the test coverage for transcripts essentially just by creating the transcripts themselves. I'm jealous I didn't think of it first ;-)
| assert out == '' | ||
|
|
||
|
|
||
| @pytest.mark.parametrize('expected, transformed', [ |
There was a problem hiding this comment.
I am not a regex guy and am fundamentally unqualified to review this code. But you really seem like you know what you are doing, so thanks for the good work. Your code modifications seem to have excellent test coverage.
|
I manually forced an update to your PR branch to pull in the small number of changes from master. As long as that happened without conflicts and all of the unit tests pass, I'll merge it in shortly. |
2 participants
This PR makes a breaking change to the format and expectations of transcript testing. The prior behavior removed whitespace before making the comparison. This new version does not, whitespace must match exactly. In addition, slash characters not intended to denote regular expressions must now be escaped to match properly.
This closes #213