Issue 29321041: Issue 2707 - Support start/end notification parameters (Closed)
|
Created:
June 23, 2015, 3:01 p.m. by Felix Dahlke Modified:
June 24, 2015, 8:53 p.m. Reviewers:
Sebastian Noack CC:
Wladimir Palant Visibility:
Public. |
DescriptionIssue 2707 - Support start/end notification parameters
Patch Set 1 #
Total comments: 9
Patch Set 2 : Fix import order #Patch Set 3 : Change date format #
MessagesTotal messages: 8
Made most sense to me to check for start/end in the parser (just like inactive), so I added some parser tests.
https://codereview.adblockplus.org/29321041/diff/29321042/sitescripts/notific... File sitescripts/notifications/parser.py (right): https://codereview.adblockplus.org/29321041/diff/29321042/sitescripts/notific... sitescripts/notifications/parser.py:20: import dateutil.parser Nit: Third party module imports go below corelib imports. https://codereview.adblockplus.org/29321041/diff/29321042/sitescripts/notific... sitescripts/notifications/parser.py:93: current[key] = dateutil.parser.parse(value) I wonder why we don't simply use a more simply format (e.g. "YYYYMMDD HH:MM") without timezone information, always assuming UTC, like we usually do?
https://codereview.adblockplus.org/29321041/diff/29321042/sitescripts/notific... File sitescripts/notifications/test/parser.py (right): https://codereview.adblockplus.org/29321041/diff/29321042/sitescripts/notific... sitescripts/notifications/test/parser.py:20: import mock Nit: Third party module imports go below corelib imports.
New patch set up that fixes the import order. https://codereview.adblockplus.org/29321041/diff/29321042/sitescripts/notific... File sitescripts/notifications/parser.py (right): https://codereview.adblockplus.org/29321041/diff/29321042/sitescripts/notific... sitescripts/notifications/parser.py:20: import dateutil.parser On 2015/06/23 15:41:36, Sebastian Noack wrote: > Nit: Third party module imports go below corelib imports. Oh, didn't even realise it's third party... Done. https://codereview.adblockplus.org/29321041/diff/29321042/sitescripts/notific... sitescripts/notifications/parser.py:93: current[key] = dateutil.parser.parse(value) On 2015/06/23 15:41:36, Sebastian Noack wrote: > I wonder why we don't simply use a more simply format (e.g. "YYYYMMDD HH:MM") > without timezone information, always assuming UTC, like we usually do? Now that I know that dateutil is third party, it'd make sense to me to avoid having to parse ISO 8601. Wladimir, you initially suggested this - do you have a strong opinion against using a simpler time format? https://codereview.adblockplus.org/29321041/diff/29321042/sitescripts/notific... File sitescripts/notifications/test/parser.py (right): https://codereview.adblockplus.org/29321041/diff/29321042/sitescripts/notific... sitescripts/notifications/test/parser.py:20: import mock On 2015/06/23 15:42:44, Sebastian Noack wrote: > Nit: Third party module imports go below corelib imports. Done.
https://codereview.adblockplus.org/29321041/diff/29321042/sitescripts/notific... File sitescripts/notifications/parser.py (right): https://codereview.adblockplus.org/29321041/diff/29321042/sitescripts/notific... sitescripts/notifications/parser.py:93: current[key] = dateutil.parser.parse(value) On 2015/06/23 15:51:43, Felix Dahlke wrote: > On 2015/06/23 15:41:36, Sebastian Noack wrote: > > I wonder why we don't simply use a more simply format (e.g. "YYYYMMDD HH:MM") > > without timezone information, always assuming UTC, like we usually do? > > Now that I know that dateutil is third party, it'd make sense to me to avoid > having to parse ISO 8601. > > Wladimir, you initially suggested this - do you have a strong opinion against > using a simpler time format? IIRC, the initial idea was to serve one static file, having all parameters checked on the client side. So using ISO date format would make sense, as it is recognized by the Date object in JavaScript. However, if we process the start and end date in Python now, a simpler format might be preferable.
https://codereview.adblockplus.org/29321041/diff/29321042/sitescripts/notific... File sitescripts/notifications/parser.py (right): https://codereview.adblockplus.org/29321041/diff/29321042/sitescripts/notific... sitescripts/notifications/parser.py:93: current[key] = dateutil.parser.parse(value) On 2015/06/23 15:58:38, Sebastian Noack wrote: > IIRC, the initial idea was to serve one static file, having all parameters > checked on the client side. So using ISO date format would make sense, as it is > recognized by the Date object in JavaScript. However, if we process the start > and end date in Python now, a simpler format might be preferable. I don't think the idea was ever having this checked on the client side - client side clocks are commonly skewed. Frankly, the ISO format is quite common and easier to "parse" for humans than something we invent (don't forget that these files will be edited manually). Can't we simply parse it via datetime.strptime()?
New patch set is up. https://codereview.adblockplus.org/29321041/diff/29321042/sitescripts/notific... File sitescripts/notifications/parser.py (right): https://codereview.adblockplus.org/29321041/diff/29321042/sitescripts/notific... sitescripts/notifications/parser.py:93: current[key] = dateutil.parser.parse(value) On 2015/06/23 17:24:00, Wladimir Palant wrote: > On 2015/06/23 15:58:38, Sebastian Noack wrote: > > IIRC, the initial idea was to serve one static file, having all parameters > > checked on the client side. So using ISO date format would make sense, as it > is > > recognized by the Date object in JavaScript. However, if we process the start > > and end date in Python now, a simpler format might be preferable. > > I don't think the idea was ever having this checked on the client side - client > side clocks are commonly skewed. > > Frankly, the ISO format is quite common and easier to "parse" for humans than > something we invent (don't forget that these files will be edited manually). > Can't we simply parse it via datetime.strptime()? I really don't think we want to parse ISO 8601 ourselves here, way too much complexity/work IMO. But we can go with a fixed format based on an ISO 8601 subset, e.g. "%Y-%m-%dT%H:M" ("2015-06-20T7:25"). Makes most sense to me - can still be parsed as ISO 8601, but we don't need a third party library to support all kinds of weird features we most likely won't need anyway. Did that.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
