Issue 29633713: Issue 6077 - Create script to modify the list of search engines dynamically (Closed)
|
Created:
Dec. 8, 2017, 3:37 p.m. by jens Modified:
Jan. 11, 2018, 1:19 p.m. CC:
René Jeschke Visibility:
Public. |
Description[repository: https://hg.adblockplus.org/mozharness/]
This patch set adds the function transform_search_engines_list() to transform_locales and extends the functionality of _transform_locale(). It filters and resorts the list of provided searchengines for each locale in order to match our requirements.
These changes depend on https://codereview.adblockplus.org/29633555/
Patch Set 1 #
Total comments: 11
Patch Set 2 : refactored _transform_locales() #
Total comments: 4
Patch Set 3 : #MessagesTotal messages: 8
On 2017/12/08 15:42:46, jens wrote: LGTM
https://codereview.adblockplus.org/29633713/diff/29633714/mozharness/abb/tran... File mozharness/abb/transform_locales.py (right): https://codereview.adblockplus.org/29633713/diff/29633714/mozharness/abb/tran... mozharness/abb/transform_locales.py:37: _I10N_PATH = os.path.join("abb-build", "l10n") This var should be named L10N_PATH :) https://codereview.adblockplus.org/29633713/diff/29633714/mozharness/abb/tran... mozharness/abb/transform_locales.py:39: _SEARCHPLUGINS_PATH = os.path.join("mobile", "locales", "searchplugins") Maybe the ("mobile","locales") part could be extracted to another variable to avoid writing twice the same string constants, e.g. _LOCALES_PATH = os.path.join("mobile", "locales"). Also, its not really clear to me why do we need the _LISTJSON_PATH + _LIST_JSON. Couldn't we have just one var (e.g. _LIST_JSON_PATH) that would also include the `list.json` filename? https://codereview.adblockplus.org/29633713/diff/29633714/mozharness/abb/tran... mozharness/abb/transform_locales.py:51: _KEY_DEF = "default" I know that having short variable names helps to be in compliance with pep8, but if I look into those keys in the code below, they are not clear to me, so I have to come here to see what they stand for, which is kinda of undesired https://codereview.adblockplus.org/29633713/diff/29633714/mozharness/abb/tran... mozharness/abb/transform_locales.py:162: _check_path_exists(search_list_path, logger) I think it makes more sense to check for the existence of the `list.json` file (_LISTJSON_PATH + _LIST_JSON) instead of the `search` folder. Also, since this check isn't locale dependent, it should be only made once (before actually calling _transform_locale) https://codereview.adblockplus.org/29633713/diff/29633714/mozharness/abb/tran... mozharness/abb/transform_locales.py:183: jsonFile.close() We could use the `with open` syntax here, to avoid the close statement https://codereview.adblockplus.org/29633713/diff/29633714/mozharness/abb/tran... mozharness/abb/transform_locales.py:245: for i in range(0, min(5, len(engine_order))): The condition here should be `for i in range(0, len(engine_order)):`. We used to have this limitation of 5 search engines, but we don't want this limitation anymore, so we would append whatever we have. Also, since we're not adding Ecosia into this piece of logic (it will be never present in engine_order), it seems that Ecosia will be added for the correct locales, but it won't be ordered properly, and it will be placed in last position https://codereview.adblockplus.org/29633713/diff/29633714/mozharness/abb/tran... mozharness/abb/transform_locales.py:366: def transform_search_engines_list(abs_mozilla_dir, obj_dir, I see that you're using here the `abs_mozilla_dir` nomenclature. On other funcions we name it `build_dir`, which now I don't think it was a good name choice :( . Anyways, I think we should name it consistently, so it can be more easily perceived that it means the same directory. I would suggest naming it just mozilla_dir, or abb_dir or project_dir (and also replacing `build_dir` occurrences). What do you think? https://codereview.adblockplus.org/29633713/diff/29633714/mozharness/abb/tran... mozharness/abb/transform_locales.py:373: jsonFile.close() We could use the `with open` syntax here, to avoid the close statement. Also, this block is pretty much the same that we have in _transform_locale. It might be better to have it as an utility function to avoid duplication of code https://codereview.adblockplus.org/29633713/diff/29633714/mozharness/abb/tran... mozharness/abb/transform_locales.py:375: chrome_path = os.path.join(abs_mozilla_dir, _I10N_PATH) Didn't really get why are you using a different chrome path here (in comparison to chrome_path = os.path.join(obj_dir, _CHROME_PATH) ) https://codereview.adblockplus.org/29633713/diff/29633714/mozharness/abb/tran... mozharness/abb/transform_locales.py:382: for loc in locales: If we moved all the logic that is currently being added also in _transform_locale to here, we could avoid lots of code duplications (e.g. creation of whitelist , whitelist_re, all_engine_ids, engine_ids, removed_engine_ids, and so on)
https://codereview.adblockplus.org/29633713/diff/29633714/mozharness/abb/tran... File mozharness/abb/transform_locales.py (right): https://codereview.adblockplus.org/29633713/diff/29633714/mozharness/abb/tran... mozharness/abb/transform_locales.py:382: for loc in locales: On 2017/12/13 10:44:42, diegocarloslima wrote: > If we moved all the logic that is currently being added also in > _transform_locale to here, we could avoid lots of code duplications (e.g. > creation of whitelist , whitelist_re, all_engine_ids, engine_ids, > removed_engine_ids, and so on) Also, I think that this should be an internal function, in order to avoid creating a further step in multi_locale_build. So the execution would be something like this: transform_locales() would have the main logic for reading the json file and it would pass to _transform_search_engines() which would reorder the search engines for each locale by rewriting the region.properties and would modify the json data structure that would be later been written back in list.json, inside transform_locales(), once _transform_search_engines() has ended
On 2017/12/13 10:55:29, diegocarloslima wrote: > https://codereview.adblockplus.org/29633713/diff/29633714/mozharness/abb/tran... > File mozharness/abb/transform_locales.py (right): > > https://codereview.adblockplus.org/29633713/diff/29633714/mozharness/abb/tran... > mozharness/abb/transform_locales.py:382: for loc in locales: > On 2017/12/13 10:44:42, diegocarloslima wrote: > > If we moved all the logic that is currently being added also in > > _transform_locale to here, we could avoid lots of code duplications (e.g. > > creation of whitelist , whitelist_re, all_engine_ids, engine_ids, > > removed_engine_ids, and so on) > > Also, I think that this should be an internal function, in order to avoid > creating a further step in multi_locale_build. So the execution would be > something like this: > > transform_locales() would have the main logic for reading the json file and it > would pass to _transform_search_engines() which would reorder the search engines > for each locale by rewriting the region.properties and would modify the json > data structure that would be later been written back in list.json, inside > transform_locales(), once _transform_search_engines() has ended added a new patch set
https://codereview.adblockplus.org/29633713/diff/29644606/mozharness/abb/tran... File mozharness/abb/transform_locales.py (right): https://codereview.adblockplus.org/29633713/diff/29644606/mozharness/abb/tran... mozharness/abb/transform_locales.py:38: _LISTJSON_PATH = os.path.join(_LOCALES_PATH, "search", "list.json") This seems to cause an error, since _LOCALES_PATH wasn't defined yet https://codereview.adblockplus.org/29633713/diff/29644606/mozharness/abb/tran... mozharness/abb/transform_locales.py:176: 2 new lines here... seems that it should be just one https://codereview.adblockplus.org/29633713/diff/29644606/mozharness/abb/tran... mozharness/abb/transform_locales.py:196: 3 new lines here... seems that it should be just one https://codereview.adblockplus.org/29633713/diff/29644606/mozharness/abb/tran... mozharness/abb/transform_locales.py:371: data['locales'].update({locale: {'default': {'visibleDefaultEngines': data['locales'] This line is too big to be in compliance with PEP8. I suggest you to put the whole code into pep8online.com to check if it meets with PEP8 requirements
On 2018/01/03 14:25:07, diegocarloslima wrote: > https://codereview.adblockplus.org/29633713/diff/29644606/mozharness/abb/tran... > File mozharness/abb/transform_locales.py (right): > > https://codereview.adblockplus.org/29633713/diff/29644606/mozharness/abb/tran... > mozharness/abb/transform_locales.py:38: _LISTJSON_PATH = > os.path.join(_LOCALES_PATH, "search", "list.json") > This seems to cause an error, since _LOCALES_PATH wasn't defined yet > > https://codereview.adblockplus.org/29633713/diff/29644606/mozharness/abb/tran... > mozharness/abb/transform_locales.py:176: > 2 new lines here... seems that it should be just one > > https://codereview.adblockplus.org/29633713/diff/29644606/mozharness/abb/tran... > mozharness/abb/transform_locales.py:196: > 3 new lines here... seems that it should be just one > > https://codereview.adblockplus.org/29633713/diff/29644606/mozharness/abb/tran... > mozharness/abb/transform_locales.py:371: data['locales'].update({locale: > {'default': {'visibleDefaultEngines': data['locales'] > This line is too big to be in compliance with PEP8. I suggest you to put the > whole code into http://pep8online.com to check if it meets with PEP8 requirements Seems like I did not upload the latest version. All the things you commented on are already fixed. I'll upload a new patch set
On 2018/01/10 14:25:06, jens wrote: > On 2018/01/03 14:25:07, diegocarloslima wrote: > > > https://codereview.adblockplus.org/29633713/diff/29644606/mozharness/abb/tran... > > File mozharness/abb/transform_locales.py (right): > > > > > https://codereview.adblockplus.org/29633713/diff/29644606/mozharness/abb/tran... > > mozharness/abb/transform_locales.py:38: _LISTJSON_PATH = > > os.path.join(_LOCALES_PATH, "search", "list.json") > > This seems to cause an error, since _LOCALES_PATH wasn't defined yet > > > > > https://codereview.adblockplus.org/29633713/diff/29644606/mozharness/abb/tran... > > mozharness/abb/transform_locales.py:176: > > 2 new lines here... seems that it should be just one > > > > > https://codereview.adblockplus.org/29633713/diff/29644606/mozharness/abb/tran... > > mozharness/abb/transform_locales.py:196: > > 3 new lines here... seems that it should be just one > > > > > https://codereview.adblockplus.org/29633713/diff/29644606/mozharness/abb/tran... > > mozharness/abb/transform_locales.py:371: data['locales'].update({locale: > > {'default': {'visibleDefaultEngines': data['locales'] > > This line is too big to be in compliance with PEP8. I suggest you to put the > > whole code into http://pep8online.com to check if it meets with PEP8 > requirements > > Seems like I did not upload the latest version. All the things you commented on > are already fixed. I'll upload a new patch set LGTM 
|
