New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix trailing colon #547
fix trailing colon #547
Conversation
Codecov Report
@@ Coverage Diff @@
## master #547 +/- ##
==========================================
+ Coverage 95.01% 95.01% +<.01%
==========================================
Files 302 302
Lines 2466 2468 +2
==========================================
+ Hits 2343 2345 +2
Misses 123 123
Continue to review full report at Codecov.
|
@@ -39,6 +39,7 @@ | |||
RE_NBSP = re.compile(u'\xa0', flags=re.UNICODE) | |||
RE_SPACES = re.compile(r'\s+') | |||
RE_TRIM_SPACES = re.compile(r'^\s+(\S.*?)\s+$') | |||
RE_TRIM_COLONS = re.compile(r'(\S.*?):*$') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we only strip from the beginning, I think \s*:+$
and .sub(r'', date_string)
make a bit more sense: we don’t find a match unless there is at least one colon, and we also remove any extra spacing there might be before the colon (I assume space after the colon would have be stripped already somewhere else, but I did not check).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume space after the colon would have be stripped already somewhere else, but I did not check
It wouldn't, and it's probably a bug... '31/07/2019: '
won't be parsed, because RE_TRIM_SPACES
assumes spaces present at both sides of the date string.
I'm not sure if it should be handled in this PR though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what's the best approach here. what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd leave it for another PR. Your approach undeniably fixes the target issue - and that's the point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Much appreciated!
Fixes #537
I added code to strip trailing colon and also a test for this code