Fixed issue with automated transcript generation via "history -t" #261
Conversation
There is now a post-processing step which escapes all "/" characters which transcript testing treats as a regex escape if there isn't a "\" to esape it.
|
@kotfu Since you wrote or modified most of the regex processing code within the transcript testing code, you may have a better idea of how to fix this corner case. But this PR does appear to fix it in a relatively simple and straightforward manner. |
Codecov Report
@@ Coverage Diff @@
## master #261 +/- ##
==========================================
- Coverage 95.19% 94.91% -0.29%
==========================================
Files 1 1
Lines 1312 1317 +5
==========================================
+ Hits 1249 1250 +1
- Misses 63 67 +4
Continue to review full report at Codecov.
|
|
Just missed adding this note before you merged. Looked at the post processing in the history transcript code. I'd like to pose a question for consideration: Do we think the transcript output would ever be too big for memory? Or to state it in another way, should we optimize for reduced memory usage or reduced i/o usage? If we think the answer to that question is no (or reduced i/o usage), then we could perhaps be more efficient in the post processing if instead of opening the transcript file and setting it to self.stdout, we created a StringIO file (a file in memory) and set that to self.stdout. We then do the replacement on the contents of the StringIO file, and then write it all out to the specified transcript file. This would save us a read and a write of the entire transcript. Let's call this alternative 1. If we think the answer to the question is yes (or reduced memory usage), then maybe we modify the post processing to open a temporary file and set it to self.stdout. We then open the temporary file for read, and the real transcript file for write, and read the lines one at a time, do the replacement, and write them to the real transcript file. Thereby never having to have the entire transcript in memory. Let's call this alternative 2. Reality is likely that nobody would notice whether we leave it as is, or do one of these other two options. One benefit I like of the current implementation is that there is no reliance on a temporary file, which I think is a drawback of alternative 2. Alternative 1 depends on the |
|
Personally I have never envisioned a situation where a transcript would be too big to fit in memory. But you bring up a good point. If that situation were ever possible, then we would have a problem. Can you envision a situation where you think that would be likely? If we call the current implementation "alternative 0", then do you have any preference as to alternatives 0, 1, or 2? They all seem like viable options with their own distinct advantages and disadvantages. I would be happy with any of them. I wouldn't be concerned about using the |
|
@kotfu Are there any outstanding issues other than the possible resolution here between alternatives 0, 1, and 2 that you would like to see addressed prior to putting out a 0.8.0 release? I was thinking of putting out a release within the next few days since we have made a lot of significant changes recently. |
|
In a time when most computers have gigabytes of memory, and our transcripts are likely plain text, I can't even think of a scenario where the transcript would be too big to fit into memory. I have no other concerns, and don't think there is any compelling reason to make a change to the current implementation of the history transcript generator feature. I say we leave it. I agree we have made a lot of changes, and I think it's a good time to put out a 0.8.0 release. |
There is now a post-processing step which escapes all "/" characters which transcript testing treats as a regex escape if there isn't a "" to escape it.
This closes #259.