Issue 29336582: Issue 2813 - Catch missing sitescripts module and return empty list on adblockplus.org (Closed)
|
Created:
Feb. 18, 2016, 11:03 a.m. by juliandoucette Modified:
Feb. 25, 2016, 2:53 p.m. CC:
Thomas Greiner Visibility:
Public. |
DescriptionIssue 2813 - Catch missing sitescripts module and return empty list on adblockplus.org
Patch Set 1 #
Total comments: 4
Patch Set 2 : Removed error name and added logging message #
Total comments: 2
Patch Set 3 : Added empty line to improve reading flow #
Total comments: 2
Patch Set 4 : Added line break to warning message #
Total comments: 2
Patch Set 5 : Fixed warning string spacing #MessagesTotal messages: 13
When you publish a patch set it's useful to paste the name (like "Patch Set 2 addressed feedback") into the message. Otherwise things can get confusing for large reviews. https://codereview.adblockplus.org/29336582/diff/29336583/filters/get_subscri... File filters/get_subscriptions.py (right): https://codereview.adblockplus.org/29336582/diff/29336583/filters/get_subscri... filters/get_subscriptions.py:25: except ImportError as error: Nit: You're not using error, so no point giving it a name. https://codereview.adblockplus.org/29336582/diff/29336583/filters/get_subscri... filters/get_subscriptions.py:26: return [] We should also log a warning message here, so that the user is aware that we've skipped grabbing the subscription list.
https://codereview.adblockplus.org/29336582/diff/29336583/filters/get_subscri... File filters/get_subscriptions.py (right): https://codereview.adblockplus.org/29336582/diff/29336583/filters/get_subscri... filters/get_subscriptions.py:25: except ImportError as error: On 2016/02/18 15:24:25, kzar wrote: > Nit: You're not using error, so no point giving it a name. Done. https://codereview.adblockplus.org/29336582/diff/29336583/filters/get_subscri... filters/get_subscriptions.py:26: return [] On 2016/02/18 15:24:25, kzar wrote: > We should also log a warning message here, so that the user is aware that we've > skipped grabbing the subscription list. Done.
https://codereview.adblockplus.org/29336582/diff/29336708/filters/get_subscri... File filters/get_subscriptions.py (right): https://codereview.adblockplus.org/29336582/diff/29336708/filters/get_subscri... filters/get_subscriptions.py:28: return [] Nit: An empty line below would improve the reading flow of that function.
https://codereview.adblockplus.org/29336582/diff/29336708/filters/get_subscri... File filters/get_subscriptions.py (right): https://codereview.adblockplus.org/29336582/diff/29336708/filters/get_subscri... filters/get_subscriptions.py:28: return [] On 2016/02/19 17:10:18, Sebastian Noack wrote: > Nit: An empty line below would improve the reading flow of that function. Done.
https://codereview.adblockplus.org/29336582/diff/29336733/filters/get_subscri... File filters/get_subscriptions.py (right): https://codereview.adblockplus.org/29336582/diff/29336733/filters/get_subscri... filters/get_subscriptions.py:27: logging.warning("Unable to import sitescripts, proceeding with empty subscriptions list") Nit: This line is too long (but I think the message is pretty good), could you wrap it something like this? logging.warning("Unable to import sitescripts, proceeding with empty " "subscriptions list.") Note: - Plus symbol was omitted deliberately - I also added a full stop
https://codereview.adblockplus.org/29336582/diff/29336733/filters/get_subscri... File filters/get_subscriptions.py (right): https://codereview.adblockplus.org/29336582/diff/29336733/filters/get_subscri... filters/get_subscriptions.py:27: logging.warning("Unable to import sitescripts, proceeding with empty subscriptions list") On 2016/02/19 17:57:14, kzar wrote: > Nit: This line is too long (but I think the message is pretty good), could you > wrap it something like this? > > logging.warning("Unable to import sitescripts, proceeding with empty " > "subscriptions list.") > > Note: > - Plus symbol was omitted deliberately > - I also added a full stop Done.
https://codereview.adblockplus.org/29336582/diff/29337551/filters/get_subscri... File filters/get_subscriptions.py (right): https://codereview.adblockplus.org/29336582/diff/29337551/filters/get_subscri... filters/get_subscriptions.py:28: "subscriptions list.") Nit: The indentation here looks wrong, the string "subscriptions list." should line up with the one above it.
https://codereview.adblockplus.org/29336582/diff/29337551/filters/get_subscri... File filters/get_subscriptions.py (right): https://codereview.adblockplus.org/29336582/diff/29337551/filters/get_subscri... filters/get_subscriptions.py:28: "subscriptions list.") On 2016/02/23 15:01:38, kzar wrote: > Nit: The indentation here looks wrong, the string "subscriptions list." should > line up with the one above it. My mistake.
LGTM
LGTM
LGTM
|
