Remove control character stripping. #926
Conversation
git/util.py
Outdated
| # END cut away invalid part | ||
| sline = sline.rstrip() | ||
| sline = self.re_ansi_escape.sub('', sline) | ||
| sline.strip() |
There was a problem hiding this comment.
strip() will not mutate the string. Thus it should be sline = sline.strip().
If it is important, do you think it's possible to have a test for it? Otherwise, maybe that line is not needed at all.
There was a problem hiding this comment.
I just saw that the line stripping was done previously as well, let's just re-add it then as in sline = sline.rstrip(). Apparently there is no test that would need it, and it's tech-debt I wouldn't ask you to pay either 😅.
There was a problem hiding this comment.
Absolutely, good catch!
uri-canva
force-pushed
the
control-characters
branch
from
65818d3 to
3b9b4fe
Compare
Codecov Report
@@ Coverage Diff @@
## master #926 +/- ##
=======================================
Coverage 93.62% 93.62%
=======================================
Files 59 59
Lines 9800 9800
=======================================
Hits 9175 9175
Misses 625 625Continue to review full report at Codecov.
|
|
Hmm, tests are failing, it is a bit different in behaviour, before if there was |
uri-canva
force-pushed
the
control-characters
branch
from
3b9b4fe to
f368852
Compare
uri-canva
force-pushed
the
control-characters
branch
from
f368852 to
1fe5bd0
Compare
|
Seems to have been a bad rebase on my part, now that it's rebased on master after the other PRs were merged it's working fine. |
git/util.py
Outdated
| # END cut away invalid part | ||
| line = line.rstrip() | ||
| line = self.re_ansi_escape.sub('', line) | ||
| line.strip() |
There was a problem hiding this comment.
Thank you! Once line = line.strip() is back I can merge it.
There was a problem hiding this comment.
Sorry, I must have dropped the change during the rebasing. Because the tests only work on master I lose track of where the commits are sometimes. 😆
|
Ah it's the stripping that breaks the tests. |
|
Changed it back to |
|
Thanks so much for your continued efforts, it's much appreciated :)! |
|
Thank you for the wonderful library! |
|
About 10 years later, I am not quite sure if it's wonderful anymore ;). Somehow now it breaks on master in a spot that seems unrelated. Fascinating. Could it be the difference between |
|
It does seem completely unrelated: it doesn't have anything to do with progress. I understand if it would break tests related to progress. But anyway definitely revert it if it's the problem! |
|
It looks like the |
|
Yeah I think rolling back makes sense, then figure out how to repro the failure on the PR with a new test, then fix it. Without a new test that fails on the PR there's no way to tell if the problem will reappear after the PR is merged. |
|
Thanks, @uri-canva , the respective commit was reverted. I agree, probably it would be beneficial to have a test for the initial motivation of this PR, i.e. the code removing too much of the line if the escape sequence appears in an unexpected spot, and then provide a fix. |
|
Oh yes, that too. I meant it mostly the other way around: none of the tests failed on this PR, but then some tests failed on master, so it would be good for that hole to be filled. |
The logic to strip away ansi control sequences only works if they're at the end of the line, if the control sequences are at the beginning of the line the whole line will be stripped away, and all progress information will be lost.