Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(198)

Issue 5172705124941824: Issue 2452 - Avoid replacing "\n" with newline in fixed strings (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 5 months ago by kzar
Modified:
4 years, 5 months ago
Visibility:
Public.

Description

Issue 2452 - Avoid replacing "\n" with newline in fixed strings

Patch Set 1 #

Total comments: 3

Patch Set 2 : Tidied up loop #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -2 lines) Patch
M cms/converters.py View 1 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 6
kzar
4 years, 5 months ago (2015-05-06 16:01:38 UTC) #1
Sebastian Noack
http://codereview.adblockplus.org/5172705124941824/diff/5629499534213120/cms/converters.py File cms/converters.py (left): http://codereview.adblockplus.org/5172705124941824/diff/5629499534213120/cms/converters.py#oldcode145 cms/converters.py:145: for i in range(len(fixed_strings)): While on it, mind making ...
4 years, 5 months ago (2015-05-06 16:24:30 UTC) #2
Sebastian Noack
http://codereview.adblockplus.org/5172705124941824/diff/5629499534213120/cms/converters.py File cms/converters.py (left): http://codereview.adblockplus.org/5172705124941824/diff/5629499534213120/cms/converters.py#oldcode145 cms/converters.py:145: for i in range(len(fixed_strings)): On 2015/05/06 16:24:30, Sebastian Noack ...
4 years, 5 months ago (2015-05-06 16:25:23 UTC) #3
kzar
Patch Set 2 : Tidied up loop http://codereview.adblockplus.org/5172705124941824/diff/5629499534213120/cms/converters.py File cms/converters.py (left): http://codereview.adblockplus.org/5172705124941824/diff/5629499534213120/cms/converters.py#oldcode145 cms/converters.py:145: for i ...
4 years, 5 months ago (2015-05-06 16:30:41 UTC) #4
Sebastian Noack
LGTM
4 years, 5 months ago (2015-05-06 16:31:52 UTC) #5
Wladimir Palant
4 years, 5 months ago (2015-05-06 16:40:49 UTC) #6
LGTM

We might want to look at other re.sub() calls to ensure that there is no issue
there.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5