Issue 30034555: Issue 7386 - Default search engine is not preserved during upgrade (Closed)
|
Created:
March 29, 2019, 12:36 p.m. by diegocarloslima Modified:
April 10, 2019, 1:15 p.m. Visibility:
Public. |
DescriptionIssue 7386 - Default search engine is not preserved during upgrade
Patch Set 1 #
Total comments: 5
MessagesTotal messages: 5
The main reason for this bug was because the id of duckduckgo changed. I also did refactor the code a bit and included some small fixes that are described in the file comments https://codereview.adblockplus.org/30034555/diff/30034556/abb-build/transform... File abb-build/transform_locales.py (right): https://codereview.adblockplus.org/30034555/diff/30034556/abb-build/transform... abb-build/transform_locales.py:58: "ddg", The id changed from duckduckgo to ddg https://codereview.adblockplus.org/30034555/diff/30034556/abb-build/transform... abb-build/transform_locales.py:62: "amazon"] Took the opportunity to also fix this, otherwise amazon wouldn't be included in some locales that don't match amazondotcom (e.g. for 'ja' locale is 'amazon-jp') https://codereview.adblockplus.org/30034555/diff/30034556/abb-build/transform... abb-build/transform_locales.py:310: generated_list_json = os.path.join(project_dir, Saving the modified list.json into the generated folder, so we don't have file modifications kept into the mercurial tracked files. That way, we avoid accidentally committing the changes in the original list.json
On 2019/03/29 12:51:01, diegocarloslima wrote: > The main reason for this bug was because the id of duckduckgo changed. I also > did refactor the code a bit and included some small fixes that are described in > the file comments > > https://codereview.adblockplus.org/30034555/diff/30034556/abb-build/transform... > File abb-build/transform_locales.py (right): > > https://codereview.adblockplus.org/30034555/diff/30034556/abb-build/transform... > abb-build/transform_locales.py:58: "ddg", > The id changed from duckduckgo to ddg > > https://codereview.adblockplus.org/30034555/diff/30034556/abb-build/transform... > abb-build/transform_locales.py:62: "amazon"] > Took the opportunity to also fix this, otherwise amazon wouldn't be included in > some locales that don't match amazondotcom (e.g. for 'ja' locale is 'amazon-jp') > > https://codereview.adblockplus.org/30034555/diff/30034556/abb-build/transform... > abb-build/transform_locales.py:310: generated_list_json = > os.path.join(project_dir, > Saving the modified list.json into the generated folder, so we don't have file > modifications kept into the mercurial tracked files. That way, we avoid > accidentally committing the changes in the original list.json The usage of empty lines is not 100% consistent throughout the whole script. I think the general idea is to have an empty line before every new comment, but there are a few place where it's not done.
On 2019/04/05 11:27:57, jens wrote: > On 2019/03/29 12:51:01, diegocarloslima wrote: > > The main reason for this bug was because the id of duckduckgo changed. I also > > did refactor the code a bit and included some small fixes that are described > in > > the file comments > > > > > https://codereview.adblockplus.org/30034555/diff/30034556/abb-build/transform... > > File abb-build/transform_locales.py (right): > > > > > https://codereview.adblockplus.org/30034555/diff/30034556/abb-build/transform... > > abb-build/transform_locales.py:58: "ddg", > > The id changed from duckduckgo to ddg > > > > > https://codereview.adblockplus.org/30034555/diff/30034556/abb-build/transform... > > abb-build/transform_locales.py:62: "amazon"] > > Took the opportunity to also fix this, otherwise amazon wouldn't be included > in > > some locales that don't match amazondotcom (e.g. for 'ja' locale is > 'amazon-jp') > > > > > https://codereview.adblockplus.org/30034555/diff/30034556/abb-build/transform... > > abb-build/transform_locales.py:310: generated_list_json = > > os.path.join(project_dir, > > Saving the modified list.json into the generated folder, so we don't have file > > modifications kept into the mercurial tracked files. That way, we avoid > > accidentally committing the changes in the original list.json > > The usage of empty lines is not 100% consistent throughout the whole script. I > think the general idea is to have an empty line before every new comment, but > there are a few place where it's not done. I tend to add an empty line when there's a line continuation just before, for me it feels a bit more readable
On 2019/04/05 11:52:35, diegocarloslima wrote: > On 2019/04/05 11:27:57, jens wrote: > > On 2019/03/29 12:51:01, diegocarloslima wrote: > > > The main reason for this bug was because the id of duckduckgo changed. I > also > > > did refactor the code a bit and included some small fixes that are described > > in > > > the file comments > > > > > > > > > https://codereview.adblockplus.org/30034555/diff/30034556/abb-build/transform... > > > File abb-build/transform_locales.py (right): > > > > > > > > > https://codereview.adblockplus.org/30034555/diff/30034556/abb-build/transform... > > > abb-build/transform_locales.py:58: "ddg", > > > The id changed from duckduckgo to ddg > > > > > > > > > https://codereview.adblockplus.org/30034555/diff/30034556/abb-build/transform... > > > abb-build/transform_locales.py:62: "amazon"] > > > Took the opportunity to also fix this, otherwise amazon wouldn't be included > > in > > > some locales that don't match amazondotcom (e.g. for 'ja' locale is > > 'amazon-jp') > > > > > > > > > https://codereview.adblockplus.org/30034555/diff/30034556/abb-build/transform... > > > abb-build/transform_locales.py:310: generated_list_json = > > > os.path.join(project_dir, > > > Saving the modified list.json into the generated folder, so we don't have > file > > > modifications kept into the mercurial tracked files. That way, we avoid > > > accidentally committing the changes in the original list.json > > > > The usage of empty lines is not 100% consistent throughout the whole script. I > > think the general idea is to have an empty line before every new comment, but > > there are a few place where it's not done. > > I tend to add an empty line when there's a line continuation just before, for me > it feels a bit more readable Okay, no objections from my side. LGTM.
LGTM though (not too deep) https://codereview.adblockplus.org/30034555/diff/30034556/abb-build/transform... File abb-build/transform_locales.py (right): https://codereview.adblockplus.org/30034555/diff/30034556/abb-build/transform... abb-build/transform_locales.py:187: unrequired line? https://codereview.adblockplus.org/30034555/diff/30034556/abb-build/transform... abb-build/transform_locales.py:267: unrequired line? 
|
||||||||||||||||||||||||||||||||||||||
