-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Exclude legacy extensions in the AMO whitelist #227
Exclude legacy extensions in the AMO whitelist #227
Conversation
Codecov Report
@@ Coverage Diff @@
## master #227 +/- ##
==========================================
+ Coverage 56.85% 56.89% +0.04%
==========================================
Files 46 46
Lines 2445 2450 +5
==========================================
+ Hits 1390 1394 +4
- Misses 1055 1056 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@acmiyaguchi These changes look fine for me. Can we get an R+ for merging it into the python_mozetl repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@acmiyaguchi this looks fine to me. Can you let me know if this is OK to merge in when you;ve had a chance please?
mozetl/taar/taar_amowhitelist.py
Outdated
@@ -64,12 +64,24 @@ def transform(self, json_data): | |||
|
|||
new_data = {} | |||
for guid in json_data.keys(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably change this to for guid, addon_data in json_data.items():
to be more succinct.
mozetl/taar/taar_amowhitelist.py
Outdated
if len(current_version_files) == 0: | ||
# Only allow webextensions | ||
continue | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this else
really needed? If we stumble upon the previous case, then we continue
. I think we can just drop the else
to reduce indentation as per our style guide (see "General practices").
mozetl/taar/taar_amowhitelist.py
Outdated
# Only allow webextensions | ||
continue | ||
|
||
rating = addon_data['ratings']['average'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's be consistent with the code above and use .get()
whenever possible, eventually catching errors and failing gracefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dexterp37 has this review covered, but it might be nice to document here what the structure of the json_data
read from s3 is.
ae521a4
to
b18466a
Compare
@fbertsch I've added a short comment regarding the JSON we're loading just above the fixture data. Other than that - I believe I've addressed all changes requested by @Dexterp37 |
can someone with write access merge this in please? |
The TAAR addon whitelist was including legacy addons by mistake. This filters the list further ensuring that an
is_webextension
flag exists and is true for each addon in the whitelist.