-
Notifications
You must be signed in to change notification settings - Fork 3
Apply version checks to add-ons and plugins in the blocklist #74
Apply version checks to add-ons and plugins in the blocklist #74
Conversation
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.
It looks good! Only a few comments :)
I personally don't like the naming style of variables/functions, but your changes are consistent with the rest...
Note: If you want to get rid of the failing tests, you can remove the pytest-sugar
package from the dev requirements.
amo2kinto/exporter.py
Outdated
if not vR.get('targetApplication'): | ||
return None | ||
|
||
for tA in vR['targetApplication']: |
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.
targetApplication = vR.get('targetApplication')
if not targetApplication:
return None
for tA in targetApplication:
....
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.
Ah, good point, thanks. :-)
amo2kinto/exporter.py
Outdated
return None | ||
|
||
for tA in vR['targetApplication']: | ||
if tA['guid'] == app_id: |
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.
There used to be a or 'guid' not in targetApplication
in the previous code. Maybe you want to do tA.get('guid')
instead (to avoid KeyError
and obtain None
)
if not versionRange.get('targetApplication'): | ||
add_plugin_item(pluginItems, item, versionRange, | ||
for versionRange in item.get('versionRange', []): | ||
if not versionRange.get('targetApplication'): |
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.
can't we use is_related_to()
here?
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.
I tried this first, but it didn't work, which is why I factored out part of is_related_to()
. The problem is you can invoke add_plugin_item
both with and without a targetApplication
. Iff you pass one, it includes information from that targetApplication
into the bit it writes out into blocklist.xml
. In other words, when I did that, the tests failed. :-)
As for |
I would be in favor of pinning |
@leplatrem can you check if this matches what you expected? :-) |
All good! |
@leplatrem could you have a look at this? python is very far from the language I'm most comfortable with, so please review mercilessly and I will try to update this PR quickly. :-)