1278104 - Create a new mozharness class to manage l10n localization of Google play desc

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

The attached patch proposes the creation of a new class. This class will manage localization for the Google play website.
We were using this code only for updating the description of the application.
Now, we are also using it to update the localization of the "what's new" content.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1278104 - Create a library to manage translation of Google play descriptions

Review commit: https://reviewboard.mozilla.org/r/57782/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57782/

Comment 2

[Jordan Lund (:jlund)]

Comment on attachment 8760061 [details]
Bug 1278104 - Create a library to manage translation of Google play descriptions

https://reviewboard.mozilla.org/r/57782/#review54742

looks good. granted you are stripping already existing impl out, I wonder if we can take advantage of the refactor and do some optimizations at the same time. sorry for the nits. feel free to push/yell at me if you feel strongly against my suggestions :)

::: testing/mozharness/mozharness/mozilla/storel10n.py:10
(Diff revision 1)
> +"""
> +import urllib2
> +import json
> +import sys
> +
> +class storel10n(object):

all of storel10n methods behave simularly. I wonder if these should just be standalone functions. e.g.

```python
all_locales_url = l10n_api_url + "api/google/listing/{channel}/"

def get_json_from_url(url, error_msg):
    # try: urlopen(url) and return json.load
    # except: print(error_msg) and sys.exit(1)
    
def get_list_locales(package_name):
    return get_json_from_url(all_locales_url.format(channel=package_name))
```

even get_mapping() can just return the load from json rather than using a class and storing it in self.mappings

::: testing/mozharness/mozharness/mozilla/storel10n.py:24
(Diff revision 1)
> +        """ Get all the translated locales supported by Google play
> +        So, locale unsupported by Google play won't be downloaded
> +        Idem for not translated locale
> +        """
> +        try:
> +            print self.all_locales_url.format(channel=package_name)

if we are going to not use mozharness here at all and instead use print(), sys.exit, and our own url download operations, rather than self.load_json_url()[0]. I don't think this should be in mozharness/mozharness/mozilla/* but instead be put in mozharness/external_tools/*

If you don want to leave this where it is, and avail of conveniences I would recommend making a standalone class that can use mozharness core but doesn't bloat `self` itself. See `class Proxxy`[1] for an example of how to do that :)

[0] https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/base/script.py#488

[1] https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/proxxy.py#12

::: testing/mozharness/mozharness/mozilla/storel10n.py:27
(Diff revision 1)
> +        """
> +        try:
> +            print self.all_locales_url.format(channel=package_name)
> +            response = urllib2.urlopen(self.all_locales_url.format(channel=package_name))
> +        except urllib2.HTTPError:
> +            print("Could not download the locale list. Channel: '" + package_name + "', URL: '" + self.all_locales_url + "'")

I'm scared of print() in 2.7. if you want to use paranthesis, can you do it everywhere and `from __future__ import print_function` ?

::: testing/mozharness/mozharness/mozilla/storel10n.py:50
(Diff revision 1)
> +        try:
> +            response = urllib2.urlopen(self.mapping_url)
> +        except urllib2.HTTPError:
> +            print("Could not download the locale mapping list. URL: '" + self.mapping_url + "'")
> +            sys.exit(1)
> +        self.mappings = json.load(response)

tiny nit: can you declare self.mappings either in an __init__ or at class level?

maybe this method should be set_mappings() as it seems to be a side effect method

::: testing/mozharness/scripts/update_apk_description.py:54
(Diff revision 1)
>              "help": "The Google play name of the app",
>          }],
>          [["--l10n-api-url"], {
>              "dest": "l10n_api_url",
>              "help": "The L10N URL",
> -            "default": "https://l10n.mozilla-community.org/~pascalc/google_play_description/"
> +            "default": "https://l10n.mozilla-community.org/stores_l10n/"

ooo, prod location :)

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8760061 [details]
Bug 1278104 - Create a library to manage translation of Google play descriptions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57782/diff/1-2/

Comment 4

[Sylvestre Ledru [:Sylvestre]]

https://reviewboard.mozilla.org/r/57782/#review54742

> all of storel10n methods behave simularly. I wonder if these should just be standalone functions. e.g.
> 
> ```python
> all_locales_url = l10n_api_url + "api/google/listing/{channel}/"
> 
> def get_json_from_url(url, error_msg):
>     # try: urlopen(url) and return json.load
>     # except: print(error_msg) and sys.exit(1)
>     
> def get_list_locales(package_name):
>     return get_json_from_url(all_locales_url.format(channel=package_name))
> ```
> 
> even get_mapping() can just return the load from json rather than using a class and storing it in self.mappings

I could do that but this would slow down everything. The locale_mapping function is using the result of get_mapping all the time.
We would have to call the file all the time. 
This is also why I used a class to store the results.
To highlight the usage of get_mapping, I renamed to load_mapping.

> if we are going to not use mozharness here at all and instead use print(), sys.exit, and our own url download operations, rather than self.load_json_url()[0]. I don't think this should be in mozharness/mozharness/mozilla/* but instead be put in mozharness/external_tools/*
> 
> If you don want to leave this where it is, and avail of conveniences I would recommend making a standalone class that can use mozharness core but doesn't bloat `self` itself. See `class Proxxy`[1] for an example of how to do that :)
> 
> [0] https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/base/script.py#488
> 
> [1] https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/proxxy.py#12

This is much better indeed, merci!

> I'm scared of print() in 2.7. if you want to use paranthesis, can you do it everywhere and `from __future__ import print_function` ?

Removed with the implementatio of your suggestion!

Comment 5

[Jordan Lund (:jlund)]

Comment on attachment 8760061 [details]
Bug 1278104 - Create a library to manage translation of Google play descriptions

https://reviewboard.mozilla.org/r/57782/#review56508

looks great! :) looking forward to patch that uses storel10n.py

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c3dd3e28c79
Create a library to manage translation of Google play descriptions r=jlund

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8c3dd3e28c79