Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1070)

Issue 29500676: Issue 5456 - Add support to specify urlFilters in the notification repository (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 3 months ago by wspee
Modified:
2 years, 1 month ago
Reviewers:
Vasily Kuznetsov
Visibility:
Public.

Description

Issue 5456 - Add support to specify urlFilters in the notification repository

Patch Set 1 #

Total comments: 2

Patch Set 2 : Addressed formatting comments #

Patch Set 3 : Changed the commit message: removed # from the issue number #

Patch Set 4 : Added tests for urlFilters #

Total comments: 2

Patch Set 5 : Use assert instead of assertEqual #

Unified diffs Side-by-side diffs Delta from patch set Stats (+18 lines, -0 lines) Patch
M sitescripts/notifications/parser.py View 1 2 1 chunk +5 lines, -0 lines 0 comments Download
M sitescripts/notifications/test/parser.py View 1 2 3 4 1 chunk +13 lines, -0 lines 0 comments Download

Messages

Total messages: 9
wspee
2 years, 3 months ago (2017-07-28 15:33:07 UTC) #1
Vasily Kuznetsov
Hi Winsley, This also looks right, except for some coding style nitpicking (see below). Let ...
2 years, 3 months ago (2017-08-03 17:08:52 UTC) #2
wspee
https://codereview.adblockplus.org/29500676/diff/29500677/sitescripts/notifications/parser.py File sitescripts/notifications/parser.py (right): https://codereview.adblockplus.org/29500676/diff/29500677/sitescripts/notifications/parser.py#newcode96 sitescripts/notifications/parser.py:96: current['urlFilters'] = ["{}^$document".format(v.upper()) On 2017/08/03 17:08:52, Vasily Kuznetsov wrote: ...
2 years, 3 months ago (2017-08-04 08:41:23 UTC) #3
Vasily Kuznetsov
Just realized now that my first example above is misindented. Should be: foo = [bar ...
2 years, 3 months ago (2017-08-04 14:23:04 UTC) #4
wspee
On 2017/08/04 14:23:04, Vasily Kuznetsov wrote: > Just realized now that my first example above ...
2 years, 1 month ago (2017-09-21 08:15:07 UTC) #5
wspee
2 years, 1 month ago (2017-09-21 08:15:14 UTC) #6
Vasily Kuznetsov
https://codereview.adblockplus.org/29500676/diff/29551582/sitescripts/notifications/test/parser.py File sitescripts/notifications/test/parser.py (right): https://codereview.adblockplus.org/29500676/diff/29551582/sitescripts/notifications/test/parser.py#newcode166 sitescripts/notifications/test/parser.py:166: self.assertEqual( These following two asserts would look even better ...
2 years, 1 month ago (2017-09-21 09:44:10 UTC) #7
wspee
https://codereview.adblockplus.org/29500676/diff/29551582/sitescripts/notifications/test/parser.py File sitescripts/notifications/test/parser.py (right): https://codereview.adblockplus.org/29500676/diff/29551582/sitescripts/notifications/test/parser.py#newcode166 sitescripts/notifications/test/parser.py:166: self.assertEqual( On 2017/09/21 09:44:10, Vasily Kuznetsov wrote: > These ...
2 years, 1 month ago (2017-09-21 10:11:34 UTC) #8
Vasily Kuznetsov
2 years, 1 month ago (2017-09-21 11:11:01 UTC) #9
LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5