Noissue - Add crowdin api config support

Noissue - Add crowdin api config support

[Jon Sonesen, 2018-02-09 23:08:01]

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.

Thanks!

https://codereview.adblockplus.org/29693633/diff/29693637/tests/crowdin_mock_...
File tests/crowdin_mock_api.py (right):

https://codereview.adblockplus.org/29693633/diff/29693637/tests/crowdin_mock_...
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?

https://codereview.adblockplus.org/29693633/diff/29693637/tests/test_translat...
File tests/test_translations.py (right):

https://codereview.adblockplus.org/29693633/diff/29693637/tests/test_translat...
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.

[Vasily Kuznetsov, 2018-03-05 17:21:33]

Hi Jon,

This looks good! I posted a few comments here and there on the details but
overall approach seems to be a clear improvement over what we had before.

Cheers,
Vasily

P.S. After I wrote the comment about the named tuples I saw that you have a
dict-based version of mock crowdin config in patch 3. I think I prefer that one
-- I would just format the dicts a bit differently to make the code nicer to
read. Something like this:

    filenode = {
        'name': translations,
        'node_type': 'file',
    }

Seems like the throw-away named tuples are just extra work...

https://codereview.adblockplus.org/29693633/diff/29701555/tests/crowdin_mock_...
File tests/crowdin_mock_api.py (right):

https://codereview.adblockplus.org/29693633/diff/29701555/tests/crowdin_mock_...
tests/crowdin_mock_api.py:64: Supported = namedtuple('Supported',
['crowdin_code'])
Maybe rename this to `SupportedLanguage`? Otherwise it's not very clear what
this is.

https://codereview.adblockplus.org/29693633/diff/29701555/tests/crowdin_mock_...
tests/crowdin_mock_api.py:82: localenode = Locale(locale, 'directory',
tuple(files))
Why are you converting it to tuple here? Seems like naturally this thing is more
of a list, no?

https://codereview.adblockplus.org/29693633/diff/29701555/tests/crowdin_mock_...
tests/crowdin_mock_api.py:87: self.info['files'] = [i._asdict() for i in
self.info['files']]
It seems like this conversion is basically part of initialization. Maybe it
makes more sense to move it into `load`?

Another thought: the named tuples look kind of neat but here you're  converting
them to dicts and then the dicts are what's actually used. Perhaps it would be
easier to just create things as dicts in the first place? We'd get rid of the
class declarations in lines 61-64, the code in `load` would be a bit more
verbose, but perhaps a bit easier to follow since you'd know which value goes
into which key of the dict, and finally the converting code here would not be
needed at all. What do you think?

https://codereview.adblockplus.org/29693633/diff/29701555/tests/test_translat...
File tests/test_translations.py (right):

https://codereview.adblockplus.org/29693633/diff/29701555/tests/test_translat...
tests/test_translations.py:37: yield zipfile.ZipFile(zip_name + '.zip')
It seems like this zipfile object is not used anywhere. Maybe we just return the
path?

https://codereview.adblockplus.org/29693633/diff/29701555/tests/test_translat...
tests/test_translations.py:38: os.remove(zip_name + '.zip')
You could put this file into a temporary directory and then you would't need to
remove it yourself. It's about the same amount of code (this fixture is
module-wide, so you can't simply use `tmpdir` and will have to use
`tmpdir_factory` instead) but seems a bit cleaner...

https://codereview.adblockplus.org/29693633/diff/29701555/tests/test_translat...
tests/test_translations.py:42: def mock_api(temp_site):
The `CrowdinMock` has a hidden dependency from the zip file that `api_zip`
fixture creates. It would be better to make it explicit by making this fixture
depend on `api_zip` (that will return the path to the zip file) and then pass
its value into `CrowdinMock`.

[Jon Sonesen, 2018-03-06 02:30:19]

Thanks for looking at this!

https://codereview.adblockplus.org/29693633/diff/29701555/tests/crowdin_mock_...
File tests/crowdin_mock_api.py (right):

https://codereview.adblockplus.org/29693633/diff/29701555/tests/crowdin_mock_...
tests/crowdin_mock_api.py:82: localenode = Locale(locale, 'directory',
tuple(files))
On 2018/03/05 17:21:32, Vasily Kuznetsov wrote:
> Why are you converting it to tuple here? Seems like naturally this thing is
more
> of a list, no?

Acknowledged.

https://codereview.adblockplus.org/29693633/diff/29701555/tests/crowdin_mock_...
tests/crowdin_mock_api.py:87: self.info['files'] = [i._asdict() for i in
self.info['files']]
On 2018/03/05 17:21:32, Vasily Kuznetsov wrote:
> It seems like this conversion is basically part of initialization. Maybe it
> makes more sense to move it into `load`?
> 
> Another thought: the named tuples look kind of neat but here you're 
converting
> them to dicts and then the dicts are what's actually used. Perhaps it would be
> easier to just create things as dicts in the first place? We'd get rid of the
> class declarations in lines 61-64, the code in `load` would be a bit more
> verbose, but perhaps a bit easier to follow since you'd know which value goes
> into which key of the dict, and finally the converting code here would not be
> needed at all. What do you think?

Acknowledged.

https://codereview.adblockplus.org/29693633/diff/29701555/tests/test_translat...
File tests/test_translations.py (right):

https://codereview.adblockplus.org/29693633/diff/29701555/tests/test_translat...
tests/test_translations.py:37: yield zipfile.ZipFile(zip_name + '.zip')
On 2018/03/05 17:21:33, Vasily Kuznetsov wrote:
> It seems like this zipfile object is not used anywhere. Maybe we just return
the
> path?

Acknowledged.

https://codereview.adblockplus.org/29693633/diff/29701555/tests/test_translat...
tests/test_translations.py:38: os.remove(zip_name + '.zip')
On 2018/03/05 17:21:33, Vasily Kuznetsov wrote:
> You could put this file into a temporary directory and then you would't need
to
> remove it yourself. It's about the same amount of code (this fixture is
> module-wide, so you can't simply use `tmpdir` and will have to use
> `tmpdir_factory` instead) but seems a bit cleaner...

Acknowledged.

https://codereview.adblockplus.org/29693633/diff/29701555/tests/test_translat...
tests/test_translations.py:42: def mock_api(temp_site):
On 2018/03/05 17:21:33, Vasily Kuznetsov wrote:
> The `CrowdinMock` has a hidden dependency from the zip file that `api_zip`
> fixture creates. It would be better to make it explicit by making this fixture
> depend on `api_zip` (that will return the path to the zip file) and then pass
> its value into `CrowdinMock`.

Acknowledged.

[Vasily Kuznetsov, 2018-03-06 11:17:37]

Hi Jon,

LAGTM! (looks almost good to me :D) Just one comment about the formatting of the
dicts in `crowdin_mock_api.load()`.

Indeed it seems that we don't really need the CrowdinMock class anymore.

Cheers,
Vasily

https://codereview.adblockplus.org/29693633/diff/29714660/tests/crowdin_mock_...
File tests/crowdin_mock_api.py (right):

https://codereview.adblockplus.org/29693633/diff/29714660/tests/crowdin_mock_...
tests/crowdin_mock_api.py:16: app.config['supported_languages'].append({
I would suggest reformatting the dicts that don't fit into a single line in this
function to use the following style:

    app.config['foobar'].append({
        'foo': bar,
        'baz': qux,
    })

This would make things consistent and improve readability.

[Jon Sonesen, 2018-03-08 19:13:10]

https://codereview.adblockplus.org/29693633/diff/29714660/tests/crowdin_mock_...
File tests/crowdin_mock_api.py (right):

https://codereview.adblockplus.org/29693633/diff/29714660/tests/crowdin_mock_...
tests/crowdin_mock_api.py:16: app.config['supported_languages'].append({
On 2018/03/06 11:17:36, Vasily Kuznetsov wrote:
> I would suggest reformatting the dicts that don't fit into a single line in
this
> function to use the following style:
> 
>     app.config['foobar'].append({
>         'foo': bar,
>         'baz': qux,
>     })
> 
> This would make things consistent and improve readability.

Done.

[Vasily Kuznetsov, 2018-03-09 12:15:48]

Hi Jon,

That's not quite the format was proposing:

- Statements that fit into one line can just be left as one line without
wrapping.
- For multiline statements 4-space indents of the inner lines with the closing
braces indented the same as the first line if often more readable than indenting
follow up lines to the level of the opening braces.
- We've also agreed some time ago that we won't use 4-space indents of the
following lines without placing the closing brace on a separate line indented
same as the first line of the statement.

I have reformatted the load function in line with this to illustrate what I
mean. Perhaps we should also update the style guide for Python to be more clear
about this to avoid further confusion.

Cheers,
Vasily

https://codereview.adblockplus.org/29693633/diff/29716704/tests/crowdin_mock_...
File tests/crowdin_mock_api.py (right):

https://codereview.adblockplus.org/29693633/diff/29716704/tests/crowdin_mock_...
tests/crowdin_mock_api.py:8: def load(rootdir, zipdir):
Here's how I would format this:

def load(rootdir, zipdir):
    app.request_log = []
    app.config['zipdir'] = zipdir
    app.config['info'] = {'files': [], 'languages': []}
    app.config['supported_languages'] = []

    for root, locales, files in os.walk(rootdir):
        for locale in locales:
            files = []
            app.config['supported_languages'].append({'crowdin_code': locale})
            app.config['info']['languages'].append({
                'code': locale,
                'can_translate': 1,
                'can_approve': 1,
            })

            for translations in os.listdir(os.path.join(root, locale)):
                files.append({'name': translations, 'node_type': 'file'})

            app.config['info']['files'].append({
                'name': locale,
                'files': files,
                'node_type': 'directory',
            })

    return app

[Jon Sonesen, 2018-03-09 18:26:35]

https://codereview.adblockplus.org/29693633/diff/29716704/tests/crowdin_mock_...
File tests/crowdin_mock_api.py (right):

https://codereview.adblockplus.org/29693633/diff/29716704/tests/crowdin_mock_...
tests/crowdin_mock_api.py:8: def load(rootdir, zipdir):
On 2018/03/09 12:15:48, Vasily Kuznetsov wrote:
> Here's how I would format this:
> 
> def load(rootdir, zipdir):
>     app.request_log = []
>     app.config['zipdir'] = zipdir
>     app.config['info'] = {'files': [], 'languages': []}
>     app.config['supported_languages'] = []
> 
>     for root, locales, files in os.walk(rootdir):
>         for locale in locales:
>             files = []
>             app.config['supported_languages'].append({'crowdin_code': locale})
>             app.config['info']['languages'].append({
>                 'code': locale,
>                 'can_translate': 1,
>                 'can_approve': 1,
>             })
> 
>             for translations in os.listdir(os.path.join(root, locale)):
>                 files.append({'name': translations, 'node_type': 'file'})
> 
>             app.config['info']['files'].append({
>                 'name': locale,
>                 'files': files,
>                 'node_type': 'directory',
>             })
> 
>     return app

Hey,

thanks, although I don't think reformatting the whole function for me was
necessary.

[Jon Sonesen, 2018-03-09 18:39:34]

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

[Vasily Kuznetsov, 2018-03-12 15:33:54]

On 2018/03/09 18:26:35, Jon Sonesen wrote:
>
https://codereview.adblockplus.org/29693633/diff/29716704/tests/crowdin_mock_...
> File tests/crowdin_mock_api.py (right):
> 
>
https://codereview.adblockplus.org/29693633/diff/29716704/tests/crowdin_mock_...
> tests/crowdin_mock_api.py:8: def load(rootdir, zipdir):
> On 2018/03/09 12:15:48, Vasily Kuznetsov wrote:
> > Here's how I would format this:
> > 
> > def load(rootdir, zipdir):
> >     app.request_log = []
> >     app.config['zipdir'] = zipdir
> >     app.config['info'] = {'files': [], 'languages': []}
> >     app.config['supported_languages'] = []
> > 
> >     for root, locales, files in os.walk(rootdir):
> >         for locale in locales:
> >             files = []
> >             app.config['supported_languages'].append({'crowdin_code':
locale})
> >             app.config['info']['languages'].append({
> >                 'code': locale,
> >                 'can_translate': 1,
> >                 'can_approve': 1,
> >             })
> > 
> >             for translations in os.listdir(os.path.join(root, locale)):
> >                 files.append({'name': translations, 'node_type': 'file'})
> > 
> >             app.config['info']['files'].append({
> >                 'name': locale,
> >                 'files': files,
> >                 'node_type': 'directory',
> >             })
> > 
> >     return app
> 
> Hey,
> 
> thanks, although I don't think reformatting the whole function for me was
> necessary.

Yeah, probably not necessary, but sometimes it's easier to show than to explain.
BTW, the last patch set (6) still doesn't really follow the convention I
proposed -- do you specifically prefer this other style?

[Vasily Kuznetsov, 2018-03-12 15:35:07]

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

[Jon Sonesen, 2018-03-12 21:14:06]

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

[Vasily Kuznetsov, 2018-03-13 17:37:38]

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!