Issue 3246 - Strip empty locale files provided by Crowdin

Issue 3246 - Strip empty locale files provided by Crowdin

[kzar, 2015-10-29 18:39:37]

Patch Set 1

[Wladimir Palant, 2015-10-29 19:12:36]

https://codereview.adblockplus.org/29329499/diff/29329500/cms/bin/translate.py
File cms/bin/translate.py (right):

https://codereview.adblockplus.org/29329499/diff/29329500/cms/bin/translate.p...
cms/bin/translate.py:268: if re.search(non_empty_file_regexp,
locale_file_contents):
I don't think that using regular expressions on JSON data is a good idea. It
makes more sense to check len(json.load(source_file)), this will also ensure
validity of the data. Positive side-effect: we can dump the JSON data formatted
the way we like it (usually means these parameters: ensure_ascii=False,
sort_keys=True, indent=2, separators=(',', ': ')).

Having to handle UTF-8 here makes things a bit more complicated unfortunately,
the simplest solution should be using the codecs module (io module is sadly
incompatible to zipfile in Python 2).

[kzar, 2015-10-30 12:03:04]

Patch Set 2 : Parse the JSON instead of using regexp

https://codereview.adblockplus.org/29329499/diff/29329500/cms/bin/translate.py
File cms/bin/translate.py (right):

https://codereview.adblockplus.org/29329499/diff/29329500/cms/bin/translate.p...
cms/bin/translate.py:268: if re.search(non_empty_file_regexp,
locale_file_contents):
On 2015/10/29 19:12:35, Wladimir Palant wrote:
> I don't think that using regular expressions on JSON data is a good idea. It
> makes more sense to check len(json.load(source_file)), this will also ensure
> validity of the data. Positive side-effect: we can dump the JSON data
formatted
> the way we like it (usually means these parameters: ensure_ascii=False,
> sort_keys=True, indent=2, separators=(',', ': ')).
> 
> Having to handle UTF-8 here makes things a bit more complicated unfortunately,
> the simplest solution should be using the codecs module (io module is sadly
> incompatible to zipfile in Python 2).

Done.

[Wladimir Palant, 2015-10-30 18:02:41]

https://codereview.adblockplus.org/29329499/diff/29329543/cms/bin/translate.py
File cms/bin/translate.py (right):

https://codereview.adblockplus.org/29329499/diff/29329543/cms/bin/translate.p...
cms/bin/translate.py:269: continue
I don't think that sweeping exceptions under the carpet is a good idea here - we
should at least warn if a file wasn't valid JSON.

[kzar, 2015-10-30 18:17:29]

Patch Set 3 : Don't catch ValueError if JSON files won't parse

https://codereview.adblockplus.org/29329499/diff/29329543/cms/bin/translate.py
File cms/bin/translate.py (right):

https://codereview.adblockplus.org/29329499/diff/29329543/cms/bin/translate.p...
cms/bin/translate.py:269: continue
On 2015/10/30 18:02:40, Wladimir Palant wrote:
> I don't think that sweeping exceptions under the carpet is a good idea here -
we
> should at least warn if a file wasn't valid JSON.

Fair point, this might hide an unrelated issue. (I was trying to handle the case
of them returning a completely empty file, but I guess we can cross that bridge
when we come to it.) I think it's better to not not catch the exception rather
than log a warning, Done.

[Wladimir Palant, 2015-11-05 13:06:34]

LGTM