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

Issue 29693633: Noissue - Add crowdin api config support

Can't Edit
Can't Publish+Mail
Start Review
1 week, 1 day ago by Jon Sonesen
11 hours, 40 minutes ago
Vasily Kuznetsov
Base URL:


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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+63 lines, -86 lines) Patch
M tests/crowdin_mock_api.py View 1 2 3 2 chunks +44 lines, -73 lines 0 comments Download
M tests/test_translations.py View 1 2 1 chunk +19 lines, -13 lines 0 comments Download


Total messages: 2
Jon Sonesen
1 week, 1 day ago (2018-02-09 23:01:38 UTC) #1
Jon Sonesen
1 week, 1 day ago (2018-02-09 23:08:01 UTC) #2
Hey vasily, 

I don't think this is ready to merge at all but wanted to get your opinion on
the design of the usage in test_translations, i believe it can be improved.


File tests/crowdin_mock_api.py (right):

tests/crowdin_mock_api.py:73: for root, locales, files in os.walk(filepath):
There are some assumptions that are made here, and should maybe be documented?
Also, I was considering changing this object to include the mockapi method, what
do you think?

File tests/test_translations.py (right):

tests/test_translations.py:39: def mock_api(temp_site):
I know this is purely cosmetic but it sort of makes more sense to me since now
it returns the app object. However, looking at this it should also probably
return the config object? But, the app.config can be accessed using the returned
mock api app object.
Sign in to reply to this message.

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