Issue 5144941147717632: Issue 2112 - Support fixed text in strings. (Closed)
DescriptionIssue 2112 - Support fixed text in strings.
Patch Set 1 #
Total comments: 11
Patch Set 2 : Addressed feedback. #
Total comments: 2
Patch Set 3 : Avoid escaping search string for fixed strings. #MessagesTotal messages: 7
http://codereview.adblockplus.org/5144941147717632/diff/5629499534213120/cms/... File cms/converters.py (right): http://codereview.adblockplus.org/5144941147717632/diff/5629499534213120/cms/... cms/converters.py:64: return "".join(self._string), self._attrs, map(lambda s: "".join(s), self._fixed_strings) Nit: ["".join(s) for s in self._fixed_strings] is shorter and easier to read than map(). http://codereview.adblockplus.org/5144941147717632/diff/5629499534213120/cms/... cms/converters.py:73: if tag not in self._whitelist: No tags should be allowed inside <fix> - meaning |if self._inside_fixed or tag not in self._whitelist| http://codereview.adblockplus.org/5144941147717632/diff/5629499534213120/cms/... cms/converters.py:89: def _append_string(self, s): Nit: we are adding to self._string a lot, this helper only applies to text data. Call it _append_text()? http://codereview.adblockplus.org/5144941147717632/diff/5629499534213120/cms/... cms/converters.py:107: whitelist = set(["a", "em", "fix", "strong"]) It makes no sense to change this set - <fix> isn't actually whitelisted, it gets special treatment inside AttributeParser. Note that this set is also used below when deciding which attributes to restore. So I'd suggest passing in the original set to AttributeParser. handle_starttag() could then go like this: if self._inside_fixed: # Unexpected tag inside a fixed string elif tag == "fix": ... elif tag in self.whitelist: ... else: # Unexpected tag in localizable string http://codereview.adblockplus.org/5144941147717632/diff/5629499534213120/cms/... cms/converters.py:140: result = re.sub(re_escape("{%d}" % (i + 1)), fixed_strings[i], result, 1) Escape the string being inserted? It looks like currently < will be turned into <.
Patch Set 2 : Addressed feedback. http://codereview.adblockplus.org/5144941147717632/diff/5629499534213120/cms/... File cms/converters.py (right): http://codereview.adblockplus.org/5144941147717632/diff/5629499534213120/cms/... cms/converters.py:64: return "".join(self._string), self._attrs, map(lambda s: "".join(s), self._fixed_strings) On 2015/03/26 20:48:56, Wladimir Palant wrote: > Nit: ["".join(s) for s in self._fixed_strings] is shorter and easier to read > than map(). Done. http://codereview.adblockplus.org/5144941147717632/diff/5629499534213120/cms/... cms/converters.py:73: if tag not in self._whitelist: On 2015/03/26 20:48:56, Wladimir Palant wrote: > No tags should be allowed inside <fix> - meaning |if self._inside_fixed or tag > not in self._whitelist| Done. http://codereview.adblockplus.org/5144941147717632/diff/5629499534213120/cms/... cms/converters.py:89: def _append_string(self, s): On 2015/03/26 20:48:56, Wladimir Palant wrote: > Nit: we are adding to self._string a lot, this helper only applies to text data. > Call it _append_text()? Done. http://codereview.adblockplus.org/5144941147717632/diff/5629499534213120/cms/... cms/converters.py:107: whitelist = set(["a", "em", "fix", "strong"]) On 2015/03/26 20:48:56, Wladimir Palant wrote: > It makes no sense to change this set - <fix> isn't actually whitelisted, it gets > special treatment inside AttributeParser. Note that this set is also used below > when deciding which attributes to restore. > > So I'd suggest passing in the original set to AttributeParser. handle_starttag() > could then go like this: > > if self._inside_fixed: > # Unexpected tag inside a fixed string > elif tag == "fix": > ... > elif tag in self.whitelist: > ... > else: > # Unexpected tag in localizable string Done. http://codereview.adblockplus.org/5144941147717632/diff/5629499534213120/cms/... cms/converters.py:140: result = re.sub(re_escape("{%d}" % (i + 1)), fixed_strings[i], result, 1) On 2015/03/26 20:48:56, Wladimir Palant wrote: > Escape the string being inserted? It looks like currently < will be turned > into <. I originally did escape fixed_strings[i] here but it seems unnecessary. I'm not entirely sure why, perhaps because of the `result = escape(result)` below?
http://codereview.adblockplus.org/5144941147717632/diff/5629499534213120/cms/... File cms/converters.py (right): http://codereview.adblockplus.org/5144941147717632/diff/5629499534213120/cms/... cms/converters.py:140: result = re.sub(re_escape("{%d}" % (i + 1)), fixed_strings[i], result, 1) On 2015/03/26 22:11:32, kzar wrote: > I originally did escape fixed_strings[i] here but it seems unnecessary. I'm not > entirely sure why, perhaps because of the `result = escape(result)` below? I see, you are inserting them before the escaping happens. Then there is another issue however... http://codereview.adblockplus.org/5144941147717632/diff/5724160613416960/cms/... File cms/converters.py (right): http://codereview.adblockplus.org/5144941147717632/diff/5724160613416960/cms/... cms/converters.py:142: result = re.sub(re_escape("{%d}" % (i + 1)), fixed_strings[i], result, 1) The string isn't escaped yes, meaning that we shouldn't escape our search string either - re.sub(r"\{%d\}" % (i + 1), ...) will do.
http://codereview.adblockplus.org/5144941147717632/diff/5724160613416960/cms/... File cms/converters.py (right): http://codereview.adblockplus.org/5144941147717632/diff/5724160613416960/cms/... cms/converters.py:142: result = re.sub(re_escape("{%d}" % (i + 1)), fixed_strings[i], result, 1) On 2015/03/27 07:29:48, Wladimir Palant wrote: > The string isn't escaped yes, meaning that we shouldn't escape our search string > either - re.sub(r"\{%d\}" % (i + 1), ...) will do. Done.
LGTM
LGTM 
|
