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

Issue 29693633: Noissue - Add crowdin api config support (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months ago by Jon Sonesen
Modified:
5 months ago
Reviewers:
Vasily Kuznetsov
Base URL:
https://hg.adblockplus.org/cms
Visibility:
Public.

Description

Noissue - Add crowdin api config support

Patch Set 1 : #

Total comments: 2

Patch Set 2 : refactor abstractions for config and the wsgi app #

Patch Set 3 : fix indentation error causing improper config load #

Patch Set 4 : implement named tuples #

Total comments: 11

Patch Set 5 : address PS4 comments #

Total comments: 2

Patch Set 6 : Address PS6 #

Patch Set 7 : Actually address PS6 comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+55 lines, -89 lines) Patch
M tests/crowdin_mock_api.py View 1 2 3 4 5 6 2 chunks +38 lines, -74 lines 0 comments Download
M tests/test_translations.py View 1 2 3 4 1 chunk +17 lines, -15 lines 0 comments Download

Messages

Total messages: 13
Jon Sonesen
6 months ago (2018-02-09 23:01:38 UTC) #1
Jon Sonesen
Hey vasily, I don't think this is ready to merge at all but wanted to ...
6 months ago (2018-02-09 23:08:01 UTC) #2
Vasily Kuznetsov
Hi Jon, This looks good! I posted a few comments here and there on the ...
5 months, 1 week ago (2018-03-05 17:21:33 UTC) #3
Jon Sonesen
Thanks for looking at this! https://codereview.adblockplus.org/29693633/diff/29701555/tests/crowdin_mock_api.py File tests/crowdin_mock_api.py (right): https://codereview.adblockplus.org/29693633/diff/29701555/tests/crowdin_mock_api.py#newcode82 tests/crowdin_mock_api.py:82: localenode = Locale(locale, 'directory', ...
5 months, 1 week ago (2018-03-06 02:30:19 UTC) #4
Vasily Kuznetsov
Hi Jon, LAGTM! (looks almost good to me :D) Just one comment about the formatting ...
5 months, 1 week ago (2018-03-06 11:17:37 UTC) #5
Jon Sonesen
https://codereview.adblockplus.org/29693633/diff/29714660/tests/crowdin_mock_api.py File tests/crowdin_mock_api.py (right): https://codereview.adblockplus.org/29693633/diff/29714660/tests/crowdin_mock_api.py#newcode16 tests/crowdin_mock_api.py:16: app.config['supported_languages'].append({ On 2018/03/06 11:17:36, Vasily Kuznetsov wrote: > I ...
5 months, 1 week ago (2018-03-08 19:13:10 UTC) #6
Vasily Kuznetsov
Hi Jon, That's not quite the format was proposing: - Statements that fit into one ...
5 months, 1 week ago (2018-03-09 12:15:48 UTC) #7
Jon Sonesen
https://codereview.adblockplus.org/29693633/diff/29716704/tests/crowdin_mock_api.py File tests/crowdin_mock_api.py (right): https://codereview.adblockplus.org/29693633/diff/29716704/tests/crowdin_mock_api.py#newcode8 tests/crowdin_mock_api.py:8: def load(rootdir, zipdir): On 2018/03/09 12:15:48, Vasily Kuznetsov wrote: ...
5 months, 1 week ago (2018-03-09 18:26:35 UTC) #8
Jon Sonesen
Thanks for reviewing. Probably updating the style guide is good. I have mostly been writing ...
5 months, 1 week ago (2018-03-09 18:39:34 UTC) #9
Vasily Kuznetsov
On 2018/03/09 18:26:35, Jon Sonesen wrote: > https://codereview.adblockplus.org/29693633/diff/29716704/tests/crowdin_mock_api.py > File tests/crowdin_mock_api.py (right): > > https://codereview.adblockplus.org/29693633/diff/29716704/tests/crowdin_mock_api.py#newcode8 ...
5 months ago (2018-03-12 15:33:54 UTC) #10
Vasily Kuznetsov
On 2018/03/09 18:39:34, Jon Sonesen wrote: > Thanks for reviewing. Probably updating the style guide ...
5 months ago (2018-03-12 15:35:07 UTC) #11
Jon Sonesen
On 2018/03/12 15:35:07, Vasily Kuznetsov wrote: > On 2018/03/09 18:39:34, Jon Sonesen wrote: > > ...
5 months ago (2018-03-12 21:14:06 UTC) #12
Vasily Kuznetsov
5 months ago (2018-03-13 17:37:38 UTC) #13
On 2018/03/12 21:14:06, Jon Sonesen wrote:
> On 2018/03/12 15:35:07, Vasily Kuznetsov wrote:
> > On 2018/03/09 18:39:34, Jon Sonesen wrote:
> > > Thanks for reviewing. Probably updating the style guide is good. I have
> mostly
> > > been writing JS though recently so I was just being bad at remembering our
> > > styles. 
> > > 
> > > Cheers
> > 
> > Yeah, I will update the style. Otherwise these formatting-related
> > back-and-forth's are quite annoying for both of us and don't really deliver
> much
> > value :/
> > 
> > Cheers
> 
> Yeah pretty much no value, also nah...I just hadn't committed my changes
> locally. So I goofed sorry lol

LGTM!
Sign in to reply to this message.

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