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

Issue 29561557: Issue 5763 - Target languages supported by Firefox (Closed)

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

Description

Issue 5763 - Target languages supported by Firefox

Patch Set 1 #

Total comments: 10

Patch Set 2 : Adapted setuptrans + addressed nits #

Patch Set 3 : Adressed Vasily's comments #

Patch Set 4 : Removed unused target_platforms key #

Total comments: 2

Patch Set 5 : Removed redundant whitespace and fixed unrelated bug #

Total comments: 1

Patch Set 6 : Fixed import_locales() #

Total comments: 5

Patch Set 7 : Fixed undefined variable, put URLS in globals, removed redundand flake8 ignores #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+91 lines, -164 lines) Patch
M build.py View 1 2 3 4 4 chunks +1 line, -5 lines 0 comments Download
M localeTools.py View 1 2 3 4 5 6 6 chunks +54 lines, -109 lines 0 comments Download
M packagerChrome.py View 1 2 3 4 5 6 6 chunks +34 lines, -48 lines 2 comments Download
M tox.ini View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 2 comments Download

Messages

Total messages: 13
Sebastian Noack
2 years, 4 months ago (2017-10-01 17:13:58 UTC) #1
tlucas
Hey Sebastian, please find the comments below. Best, Tristan https://codereview.adblockplus.org/29561557/diff/29561558/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29561557/diff/29561558/localeTools.py#newcode64 localeTools.py:64: ...
2 years, 4 months ago (2017-10-02 09:25:43 UTC) #2
Vasily Kuznetsov
Hi Sebastian, It looks pretty good to me other than a couple of rather philosophical ...
2 years, 4 months ago (2017-10-02 21:13:32 UTC) #3
Vasily Kuznetsov
On 2017/10/02 21:13:32, Vasily Kuznetsov wrote: > This loop with a regexp search and then ...
2 years, 4 months ago (2017-10-02 21:14:28 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/29561557/diff/29561558/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29561557/diff/29561558/localeTools.py#newcode64 localeTools.py:64: 'es-419', On 2017/10/02 09:25:43, tlucas wrote: > iirc, removing ...
2 years, 4 months ago (2017-10-02 23:01:33 UTC) #5
tlucas
Hey Sebastian, looks pretty good ( and actually takes a lot of work from 5751 ...
2 years, 4 months ago (2017-10-04 09:37:39 UTC) #6
tlucas
By the way, this just popped into my mind: Have you tried running "setuptrans", "translate", ...
2 years, 4 months ago (2017-10-04 10:23:51 UTC) #7
Sebastian Noack
On 2017/10/04 10:23:51, tlucas wrote: > By the way, this just popped into my mind: ...
2 years, 4 months ago (2017-10-04 21:33:51 UTC) #8
tlucas
LGTM
2 years, 4 months ago (2017-10-05 09:33:52 UTC) #9
Sebastian Noack
https://codereview.adblockplus.org/29561557/diff/29565791/packagerChrome.py File packagerChrome.py (left): https://codereview.adblockplus.org/29561557/diff/29565791/packagerChrome.py#oldcode246 packagerChrome.py:246: if not os.path.exists(sourceFile) or os.path.exists(incompleteMarker): I didn't care to ...
2 years, 4 months ago (2017-10-05 19:01:44 UTC) #10
Vasily Kuznetsov
Hi Sebastian, I wouldn't be able to confirm the validity of some of the logic ...
2 years, 4 months ago (2017-10-05 20:26:03 UTC) #11
Sebastian Noack
https://codereview.adblockplus.org/29561557/diff/29565791/localeTools.py File localeTools.py (right): https://codereview.adblockplus.org/29561557/diff/29565791/localeTools.py#newcode246 localeTools.py:246: data = urllib2.urlopen('http://www.mozilla.org/en-US/firefox/all.html').read() On 2017/10/05 20:26:03, Vasily Kuznetsov wrote: ...
2 years, 4 months ago (2017-10-05 20:58:39 UTC) #12
tlucas
2 years, 4 months ago (2017-10-06 08:53:54 UTC) #13
Hey Sebastian,

still LGTM.

https://codereview.adblockplus.org/29561557/diff/29565833/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29561557/diff/29565833/packagerChrome.py#n...
packagerChrome.py:221: parts = sourceFile.split(os.path.sep)
On 2017/10/05 20:58:38, Sebastian Noack wrote:
> I just noticed that the variable "parts" is used below. I didn't notice
before,
> because it's only used in the branch for legacy Gecko extensions below.
> 
> @Tristan: Feel free to remove this variable again when removing support for
> legacy Gecko extensions.

Acknowledged.

https://codereview.adblockplus.org/29561557/diff/29565833/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29561557/diff/29565833/tox.ini#newcode12
tox.ini:12: packagerChrome.py :
A101,A103,A104,A107,A108,A111,A112,A302,E501,E711,F841,N802,N803,N806
On 2017/10/05 20:58:39, Sebastian Noack wrote:
> These ignores are no longer necessary.
> 
> @Tristan: It might be worth to check again whether we can reduce this list
even
> further after your changes.

Good idea, will do.
Sign in to reply to this message.

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