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

Issue 29517660: Issue 5477 - Import everything from imported locales (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 6 months ago by tlucas
Modified:
2 years, 6 months ago
Visibility:
Public.

Description

Issue 5477 - Import everything from imported locales

Patch Set 1 #

Patch Set 2 : #

Total comments: 2

Patch Set 3 : Adressing comment from Sebastian #

Total comments: 2

Patch Set 4 : Moving access key removal #

Total comments: 2

Patch Set 5 : Further simplifying #

Total comments: 10

Patch Set 6 : #

Total comments: 8

Patch Set 7 : #

Patch Set 8 : #

Total comments: 12

Patch Set 9 : #

Patch Set 10 : Minor docstring changes #

Total comments: 4

Patch Set 11 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+35 lines, -16 lines) Patch
M packagerChrome.py View 1 2 3 4 5 6 7 8 9 10 4 chunks +32 lines, -13 lines 0 comments Download
M packagerEdge.py View 1 1 chunk +1 line, -1 line 0 comments Download
M packagerSafari.py View 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 27
tlucas
Patch Set 1: * Let every additional information when importing translations in packagerChrome/Safari be included ...
2 years, 6 months ago (2017-08-16 14:22:32 UTC) #1
Sebastian Noack
It seems you missed at least one usage of importGeckoLocales (in packagerEdge.py). Also this code ...
2 years, 6 months ago (2017-08-16 15:35:00 UTC) #2
tlucas
Patch Set 2 On 2017/08/16 15:35:00, Sebastian Noack wrote: > It seems you missed at ...
2 years, 6 months ago (2017-08-16 18:33:15 UTC) #3
Sebastian Noack
On 2017/08/16 18:33:15, tlucas wrote: > As for packagerGecko -> gecko-webext does not use it ...
2 years, 6 months ago (2017-08-17 16:32:34 UTC) #4
tlucas
Patch Set 3 * Removing lambdas * Moving additional function-defs for consistency https://codereview.adblockplus.org/29517660/diff/29517743/packagerChrome.py File packagerChrome.py ...
2 years, 6 months ago (2017-08-18 07:34:15 UTC) #5
Sebastian Noack
https://codereview.adblockplus.org/29517660/diff/29519555/packagerChrome.py File packagerChrome.py (left): https://codereview.adblockplus.org/29517660/diff/29519555/packagerChrome.py#oldcode260 packagerChrome.py:260: match = re.search(r'^(.*?)\s*\(&.\)$', value) The logic to strip the ...
2 years, 6 months ago (2017-08-18 07:44:28 UTC) #6
tlucas
Patch Set 4 Moving access key-removal to possible gecko-style translations only https://codereview.adblockplus.org/29517660/diff/29519555/packagerChrome.py File packagerChrome.py (left): ...
2 years, 6 months ago (2017-08-18 12:59:37 UTC) #7
Sebastian Noack
https://codereview.adblockplus.org/29517660/diff/29519608/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29519608/packagerChrome.py#newcode287 packagerChrome.py:287: set_translation(data, key, value, sourceData, stringID) It seems the logic ...
2 years, 6 months ago (2017-08-18 13:54:49 UTC) #8
tlucas
Patch Set 5 Further simplified the import_locales code, moving get_ and set_ logic to top-level ...
2 years, 6 months ago (2017-08-18 14:35:42 UTC) #9
Sebastian Noack
https://codereview.adblockplus.org/29517660/diff/29519620/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29519620/packagerChrome.py#newcode195 packagerChrome.py:195: info from the source dict""" As per PEP-257, the ...
2 years, 6 months ago (2017-08-18 14:54:00 UTC) #10
tlucas
Patch Set 6 Simplified arguments (and func-names) of importing logic, adhered to Sebastian's comments on ...
2 years, 6 months ago (2017-08-18 15:10:35 UTC) #11
Sebastian Noack
https://codereview.adblockplus.org/29517660/diff/29519624/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29519624/packagerChrome.py#newcode205 packagerChrome.py:205: """only set {'message': value} in data-dictionary, after stripping You ...
2 years, 6 months ago (2017-08-18 15:26:14 UTC) #12
tlucas
Patch Set 7 * Consistency in function names * Comment- / docstring-optimization https://codereview.adblockplus.org/29517660/diff/29519624/packagerChrome.py File packagerChrome.py ...
2 years, 6 months ago (2017-08-18 15:34:18 UTC) #13
Sebastian Noack
LGTM
2 years, 6 months ago (2017-08-18 15:36:43 UTC) #14
tlucas
Patch Set 8 Sebastian figured out that import_string_webext could simply assign source to the target[key], ...
2 years, 6 months ago (2017-08-18 15:53:52 UTC) #15
Sebastian Noack
LGTM
2 years, 6 months ago (2017-08-18 15:55:02 UTC) #16
Vasily Kuznetsov
Hi Tristan, It looks good to me and I just have two stylistic comments, one ...
2 years, 6 months ago (2017-08-21 10:43:19 UTC) #17
Wladimir Palant
https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py#newcode203 packagerChrome.py:203: # Remove access keys from possible gecko-style Nit: Gecko ...
2 years, 6 months ago (2017-08-21 10:57:21 UTC) #18
tlucas
Hey guys, thank you for the comments! Vasily, please find the discussion in the docstring-comment. ...
2 years, 6 months ago (2017-08-21 11:33:01 UTC) #19
Vasily Kuznetsov
https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py#newcode199 packagerChrome.py:199: """Only sets {'message': value} in data-dictionary, after stripping On ...
2 years, 6 months ago (2017-08-21 11:53:15 UTC) #20
tlucas
Patch Set 9 * removed unnecessary newlines * Edited docstrings according to PEP-257 https://codereview.adblockplus.org/29517660/diff/29519632/packagerChrome.py File ...
2 years, 6 months ago (2017-08-21 12:28:51 UTC) #21
tlucas
Patch Set 10 Changed docstrings to be imperative with a full-stop.
2 years, 6 months ago (2017-08-21 13:11:15 UTC) #22
Wladimir Palant
LGTM (one nit below) https://codereview.adblockplus.org/29517660/diff/29522617/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29522617/packagerChrome.py#newcode194 packagerChrome.py:194: """Import source-dict into data.""" Nit: ...
2 years, 6 months ago (2017-08-21 13:59:07 UTC) #23
Sebastian Noack
https://codereview.adblockplus.org/29517660/diff/29522617/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29522617/packagerChrome.py#newcode256 packagerChrome.py:256: Nit: The blank line here seems out of place.
2 years, 6 months ago (2017-08-22 07:38:11 UTC) #24
tlucas
Patch Set 11 Rephrase a doscstring / remove a newline https://codereview.adblockplus.org/29517660/diff/29522617/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29517660/diff/29522617/packagerChrome.py#newcode194 ...
2 years, 6 months ago (2017-08-22 08:00:50 UTC) #25
Wladimir Palant
LGTM
2 years, 6 months ago (2017-08-22 08:02:20 UTC) #26
Sebastian Noack
2 years, 6 months ago (2017-08-22 08:11:32 UTC) #27
LGTM
Sign in to reply to this message.

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