Issue 29324503: Issue 2936 - Support syntax for duplicate translatable strings (Closed)
|
Created:
Aug. 21, 2015, 9:47 a.m. by kzar Modified:
Aug. 24, 2015, 3:47 p.m. CC:
saroyanm Visibility:
Public. |
DescriptionIssue 2936 - Support syntax for duplicate translatable strings
Patch Set 1 #
Total comments: 4
Patch Set 2 : Remove misleading variable #
Total comments: 7
Patch Set 3 : Re-do logic for storing "seen strings" #Patch Set 4 : Store comment for previously seen strings #MessagesTotal messages: 9
Patch Set 1
https://codereview.adblockplus.org/29324503/diff/29324504/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29324503/diff/29324504/cms/converters.py#n... cms/converters.py:137: duplicate_string = not default Nit: I find this variable name kind misleading. The string isn't necessarily duplicated but the source translation is merely omitted if it is set to True. How about simply checking for |not default| below? https://codereview.adblockplus.org/29324503/diff/29324504/cms/converters.py#n... cms/converters.py:206: if default: Nit: If I understand the code correctly default is never None, but always a string. So this check seems to be unnecessary.
Patch Set 2 : Remove misleading variable https://codereview.adblockplus.org/29324503/diff/29324504/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29324503/diff/29324504/cms/converters.py#n... cms/converters.py:137: duplicate_string = not default On 2015/08/23 14:24:12, Sebastian Noack wrote: > Nit: I find this variable name kind misleading. The string isn't necessarily > duplicated but the source translation is merely omitted if it is set to True. > How about simply checking for |not default| below? Done. https://codereview.adblockplus.org/29324503/diff/29324504/cms/converters.py#n... cms/converters.py:206: if default: On 2015/08/23 14:24:12, Sebastian Noack wrote: > Nit: If I understand the code correctly default is never None, but always a > string. So this check seems to be unnecessary. I found that default is actually sometimes None during testing.
https://codereview.adblockplus.org/29324503/diff/29324516/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29324503/diff/29324516/cms/converters.py#n... cms/converters.py:142: default = self._params["localedata"][name] This logic is wrong, the default value should never be localized - it's the English string specified in the page source. As you have it now, placeholders will be messed up. https://codereview.adblockplus.org/29324503/diff/29324516/cms/converters.py#n... cms/converters.py:147: self._params["localedata"][name] = default 1) This should be using localedata parameter instead of self._params["localedata"]. 2) I don't think that messing up localization data is a good approach, e.g. because this localization data will be reused by other files and lead to strange errors. Also, it would be a mess with placeholders specified differently in the default value and in localization files. Why not add some property for string defaults we've seen already? IMHO the logic here should be: if default: # Maybe warn if the same default value is specified multiple times? self._seen_default[(page, name)] = default else: default = self._seen_default[(page, name)] # might throw That's it, completely independent of which locale we are processing. https://codereview.adblockplus.org/29324503/diff/29324516/cms/converters.py#n... cms/converters.py:162: if default: Why add this check? The string is still there, regardless of whether a default value has been specified.
Patch Set 3 : Re-do logic for storing "seen strings" https://codereview.adblockplus.org/29324503/diff/29324516/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29324503/diff/29324516/cms/converters.py#n... cms/converters.py:142: default = self._params["localedata"][name] On 2015/08/24 11:07:17, Wladimir Palant wrote: > This logic is wrong, the default value should never be localized - it's the > English string specified in the page source. As you have it now, placeholders > will be messed up. Acknowledged. https://codereview.adblockplus.org/29324503/diff/29324516/cms/converters.py#n... cms/converters.py:147: self._params["localedata"][name] = default On 2015/08/24 11:07:17, Wladimir Palant wrote: > 1) This should be using localedata parameter instead of > self._params["localedata"]. > > 2) I don't think that messing up localization data is a good approach, e.g. > because this localization data will be reused by other files and lead to strange > errors. Also, it would be a mess with placeholders specified differently in the > default value and in localization files. > > Why not add some property for string defaults we've seen already? IMHO the logic > here should be: > > if default: > # Maybe warn if the same default value is specified multiple times? > self._seen_default[(page, name)] = default > else: > default = self._seen_default[(page, name)] # might throw > > That's it, completely independent of which locale we are processing. Done. https://codereview.adblockplus.org/29324503/diff/29324516/cms/converters.py#n... cms/converters.py:162: if default: On 2015/08/24 11:07:17, Wladimir Palant wrote: > Why add this check? The string is still there, regardless of whether a default > value has been specified. Well as a consumer of this callback I would be surprised to receive multiple callbacks for the same string. Also we would be missing the string comment after the first callback.
https://codereview.adblockplus.org/29324503/diff/29324516/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29324503/diff/29324516/cms/converters.py#n... cms/converters.py:162: if default: On 2015/08/24 14:29:45, kzar wrote: > Well as a consumer of this callback I would be surprised to receive multiple > callbacks for the same string. That's something one has to count on anyway. > Also we would be missing the string comment after > the first callback. True, we can save the comment as well.
Patch Set 4 : Store comment for previously seen strings
LGTM I had a few nits but these are already present in the original code.
LGTM 
|
