Issue 29334171: Issue 3410 - Added date sort / format for press releases (Closed)
|
Created:
Jan. 20, 2016, 4:58 p.m. by juliandoucette Modified:
Feb. 10, 2016, 1:39 p.m. CC:
Thomas Greiner Visibility:
Public. |
DescriptionIssue 3410 - Added date sort / format for press releases
- I renamed the old datesort filter and updated the team template to avoid confusion
- I wrote a new datesort that sorts and mutates the date inside the collection argument based on an inputFormat and outputFormat
- I updated the press page to use the new datesort
I wasn't sure about the filter names here. I'm open to suggestion.
Patch Set 1 #
Total comments: 14
Patch Set 2 : Fixed rename, typos, and function names #
Total comments: 20
Patch Set 3 : Changing sort filters and utf8 charaters in release hrefs #
Total comments: 4
Patch Set 4 : Changed humanize_dates to format a single date and removed space before argument list #
Total comments: 4
Patch Set 5 : Fixed import #Patch Set 6 : Changed teamsort to be consistent with fixed import #
Total comments: 2
Patch Set 7 : Fixed datetime call #
MessagesTotal messages: 19
Added Sebastian to CC.
https://codereview.adblockplus.org/29334171/diff/29334172/filters/datesort.py File filters/datesort.py (right): https://codereview.adblockplus.org/29334171/diff/29334172/filters/datesort.py... filters/datesort.py:8: return sorted(collection, key=lambda date: datetime.strptime(date[0], outputFormat)) The items need to be sorted in descending order https://codereview.adblockplus.org/29334171/diff/29334172/filters/rndsortbyda... File filters/rndsortbydate.py (right): https://codereview.adblockplus.org/29334171/diff/29334172/filters/rndsortbyda... filters/rndsortbydate.py:1: please use "hg rename" when renaming the files, so it will be easy to look for changes. https://codereview.adblockplus.org/29334171/diff/29334172/filters/rndsortbyda... filters/rndsortbydate.py:1: no need to use short form of the words and abbreviation in file names. ex.: randomsortbydate https://codereview.adblockplus.org/29334171/diff/29334172/includes/press/rele... File includes/press/releases.tmpl (right): https://codereview.adblockplus.org/29334171/diff/29334172/includes/press/rele... includes/press/releases.tmpl:1: No new line needed in the beginning of the files. https://codereview.adblockplus.org/29334171/diff/29334172/includes/press/rele... includes/press/releases.tmpl:4: ["2015-09-30", "en", "First Free Ad Blocker on the Apple App Store Arrives; The Adblock Autumn: Adblock Plus for iOS 2-nd from ABP this Month", "2015-09-30-ABP_for_iOSApp-WEB.pdf"], This item is duplication. https://codereview.adblockplus.org/29334171/diff/29334172/includes/press/rele... includes/press/releases.tmpl:7: ["2015-09-08", "en", "Adblock Plus beats Apple to the punch; Ships Adblock Browser ahead of iOS 9", "2015-09-08-AdblockBrowserforiOSLaunch(POST]-WEB.pdf"], The link is broken because of ")" character is replaced by "]". https://codereview.adblockplus.org/29334171/diff/29334172/includes/press/rele... includes/press/releases.tmpl:8: ["2015-09-08", "en", "Adblock Plus Escapes Exile; Returns to Google Play Store", "2015-09-08-Adblock BrowserforAndroid(POST]-WEB.pdf"], The link is broken because of ")" character is replaced by "]".
I addressed all of the issues marked. I'm not sure how to best address the UTF8 issue. Do you have any suggestions Sebastian? https://codereview.adblockplus.org/29334171/diff/29334172/filters/datesort.py File filters/datesort.py (right): https://codereview.adblockplus.org/29334171/diff/29334172/filters/datesort.py... filters/datesort.py:8: return sorted(collection, key=lambda date: datetime.strptime(date[0], outputFormat)) On 2016/01/22 11:58:14, saroyanm wrote: > The items need to be sorted in descending order Done. https://codereview.adblockplus.org/29334171/diff/29334172/filters/rndsortbyda... File filters/rndsortbydate.py (right): https://codereview.adblockplus.org/29334171/diff/29334172/filters/rndsortbyda... filters/rndsortbydate.py:1: On 2016/01/22 11:58:14, saroyanm wrote: > no need to use short form of the words and abbreviation in file names. > ex.: randomsortbydate Done. https://codereview.adblockplus.org/29334171/diff/29334172/filters/rndsortbyda... filters/rndsortbydate.py:1: On 2016/01/22 11:58:14, saroyanm wrote: > please use "hg rename" when renaming the files, so it will be easy to look for > changes. Done. https://codereview.adblockplus.org/29334171/diff/29334172/includes/press/rele... File includes/press/releases.tmpl (right): https://codereview.adblockplus.org/29334171/diff/29334172/includes/press/rele... includes/press/releases.tmpl:1: On 2016/01/22 11:58:14, saroyanm wrote: > No new line needed in the beginning of the files. Done. https://codereview.adblockplus.org/29334171/diff/29334172/includes/press/rele... includes/press/releases.tmpl:4: ["2015-09-30", "en", "First Free Ad Blocker on the Apple App Store Arrives; The Adblock Autumn: Adblock Plus for iOS 2-nd from ABP this Month", "2015-09-30-ABP_for_iOSApp-WEB.pdf"], On 2016/01/22 11:58:14, saroyanm wrote: > This item is duplication. Done. https://codereview.adblockplus.org/29334171/diff/29334172/includes/press/rele... includes/press/releases.tmpl:7: ["2015-09-08", "en", "Adblock Plus beats Apple to the punch; Ships Adblock Browser ahead of iOS 9", "2015-09-08-AdblockBrowserforiOSLaunch(POST]-WEB.pdf"], On 2016/01/22 11:58:14, saroyanm wrote: > The link is broken because of ")" character is replaced by "]". Done. My mistake. https://codereview.adblockplus.org/29334171/diff/29334172/includes/press/rele... includes/press/releases.tmpl:8: ["2015-09-08", "en", "Adblock Plus Escapes Exile; Returns to Google Play Store", "2015-09-08-Adblock BrowserforAndroid(POST]-WEB.pdf"], On 2016/01/22 11:58:14, saroyanm wrote: > The link is broken because of ")" character is replaced by "]". Done.
https://codereview.adblockplus.org/29334171/diff/29334505/filters/humandates.py File filters/humandates.py (right): https://codereview.adblockplus.org/29334171/diff/29334505/filters/humandates.... filters/humandates.py:1: Detail: No new line needed in the beginning of the files. https://codereview.adblockplus.org/29334171/diff/29334505/filters/humandates.... filters/humandates.py:4: def humandates(collection, inputFormat='%Y-%m-%d', outputFormat='%B %d, %Y'): Not really sure if naming this function as human date really makes sense, because you also pass the output format as parameter, so this can be changed anytime. I think previous name was better "datesort". https://codereview.adblockplus.org/29334171/diff/29334505/filters/randomsortb... File filters/randomsortbydate.py (right): https://codereview.adblockplus.org/29334171/diff/29334505/filters/randomsortb... filters/randomsortbydate.py:1: Detail: No new line needed in the beginning of the files. https://codereview.adblockplus.org/29334171/diff/29334505/includes/press/rele... File includes/press/releases.tmpl (right): https://codereview.adblockplus.org/29334171/diff/29334505/includes/press/rele... includes/press/releases.tmpl:14: ["2015-09-10", "de", "Adblock Browser aus dem Exil entlassen - endlich zurück im Google Play Store!", "2015-09-10-Adblock_Browser_für_Android-DE.pdf"], Can you please change link to PDF file to -> 2015-09-10-Adblock_Browser_fur_Android-DE.pdf (it's leftover from initial implementation) https://codereview.adblockplus.org/29334171/diff/29334505/includes/press/rele... includes/press/releases.tmpl:15: ["2015-09-08", "de", "Adblock Plus schneller dran als Apple - Adblock Browser kommt noch vor iOS 9!", "2015-09-08-Adblock_Browser_für_iOS_Launch-DE.pdf"], Can you please change link to PDF file to -> 2015-09-08-Adblock_Browser_fur_iOS_Launch-DE.pdf (it's leftover from initial implementation) https://codereview.adblockplus.org/29334171/diff/29334505/includes/press/rele... includes/press/releases.tmpl:16: ["2015-09-08", "de", "Adblock Browser aus dem Exil entlassen - endlich zurück im Google Play Store!", "2015-09-08-Adblock_Browser_für_Android-DE.pdf"], Can you please change link to PDF file to -> 2015-09-08-Adblock_Browser_fur_Android-DE.pdf (it's leftover from initial implementation)
On 2016/01/25 16:19:56, juliandoucette wrote:
> I addressed all of the issues marked. I'm not sure how to best address the
UTF8
> issue. Do you have any suggestions Sebastian?
I think you can use 'raise ValueError("")' to raise the error
https://hg.adblockplus.org/web.adblockplus.org/file/tip/filters/inline_file.p...
To keep it simple I think we can check if the file name string in the list
contains ASCII character or not.
On 2016/02/01 16:53:53, saroyanm wrote:
> On 2016/01/25 16:19:56, juliandoucette wrote:
> > I addressed all of the issues marked. I'm not sure how to best address the
> UTF8
> > issue. Do you have any suggestions Sebastian?
>
> I think you can use 'raise ValueError("")' to raise the error
>
https://hg.adblockplus.org/web.adblockplus.org/file/tip/filters/inline_file.p...
>
>
> To keep it simple I think we can check if the file name string in the list
> contains ASCII character or not.
I have no idea what issue you are talking about. Can you please comment inline
for some context?
https://codereview.adblockplus.org/29334171/diff/29334505/filters/humandates.py
File filters/humandates.py (right):
https://codereview.adblockplus.org/29334171/diff/29334505/filters/humandates....
filters/humandates.py:5: if inputFormat != outputFormat:
This check is redundant. No need for premature micro-optimizations here.
https://codereview.adblockplus.org/29334171/diff/29334505/filters/humandates....
filters/humandates.py:7: item[0] = datetime.strftime(datetime.strptime(item[0],
inputFormat), outputFormat)
A filter changing it's input in-place is quite unexpected ...and a footgun.
https://codereview.adblockplus.org/29334171/diff/29334505/filters/randomsortb...
File filters/randomsortbydate.py (right):
https://codereview.adblockplus.org/29334171/diff/29334505/filters/randomsortb...
filters/randomsortbydate.py:1:
It seems the changes in here (except for the function name) are unrelated and
unnecessary. I'm not even sure whether they improve the code quality.
https://codereview.adblockplus.org/29334171/diff/29334505/includes/press/rele...
File includes/press/releases.tmpl (right):
https://codereview.adblockplus.org/29334171/diff/29334505/includes/press/rele...
includes/press/releases.tmpl:22: ] | humandates %}
Nit: Please call the filter where you output the data not where you define them.
This filter is about data representation, not about data structure.
On 2016/02/01 18:01:23, Sebastian Noack wrote:
> On 2016/02/01 16:53:53, saroyanm wrote:
> > On 2016/01/25 16:19:56, juliandoucette wrote:
> > > I addressed all of the issues marked. I'm not sure how to best address the
> > UTF8
> > > issue. Do you have any suggestions Sebastian?
> >
> > I think you can use 'raise ValueError("")' to raise the error
> >
>
https://hg.adblockplus.org/web.adblockplus.org/file/tip/filters/inline_file.p...
> >
> >
> > To keep it simple I think we can check if the file name string in the list
> > contains ASCII character or not.
>
> I have no idea what issue you are talking about. Can you please comment inline
> for some context?
Whenever static file which contains non ASCII character in the filename is being
added static content generation fails with the error:
"UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 13:
ordinal not in range(128)"
So the thing is that the error is not being shown when we run test server
("runserver.py").
While I asked Ben do not create PDF files which contains non ASCII characters I
was thinking it would make also sense to implement a check to be sure that
whenever non developer will try to run the test server will see an error at
least.
It was a comment suggestion in the ticket, but not mandatory change.
Another solution could be to check the static filenames in "runserver.py", if
you think that it make more sense I'll create a ticket for that.
On 2016/02/02 09:29:03, saroyanm wrote:
> On 2016/02/01 18:01:23, Sebastian Noack wrote:
> > On 2016/02/01 16:53:53, saroyanm wrote:
> > > On 2016/01/25 16:19:56, juliandoucette wrote:
> > > > I addressed all of the issues marked. I'm not sure how to best address
the
> > > UTF8
> > > > issue. Do you have any suggestions Sebastian?
> > >
> > > I think you can use 'raise ValueError("")' to raise the error
> > >
> >
>
https://hg.adblockplus.org/web.adblockplus.org/file/tip/filters/inline_file.p...
> > >
> > >
> > > To keep it simple I think we can check if the file name string in the list
> > > contains ASCII character or not.
> >
> > I have no idea what issue you are talking about. Can you please comment
inline
> > for some context?
>
> Whenever static file which contains non ASCII character in the filename is
being
> added static content generation fails with the error:
> "UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 13:
> ordinal not in range(128)"
>
> So the thing is that the error is not being shown when we run test server
> ("runserver.py").
> While I asked Ben do not create PDF files which contains non ASCII characters
I
> was thinking it would make also sense to implement a check to be sure that
> whenever non developer will try to run the test server will see an error at
> least.
> It was a comment suggestion in the ticket, but not mandatory change.
>
>
> Another solution could be to check the static filenames in "runserver.py", if
> you think that it make more sense I'll create a ticket for that.
Yeah, the testserver should behave consistently with the page generation. So
it's unrelated of the changes here, but should rather be addressed in the CMS.
However, first we should figure out what the difference is that causes this
inconsistency. (A full trace might be helpful). But let's discuss that in an
separate issue.
On 2016/02/02 09:55:50, Sebastian Noack wrote:
> On 2016/02/02 09:29:03, saroyanm wrote:
> > On 2016/02/01 18:01:23, Sebastian Noack wrote:
> > > On 2016/02/01 16:53:53, saroyanm wrote:
> > > > On 2016/01/25 16:19:56, juliandoucette wrote:
> > > > > I addressed all of the issues marked. I'm not sure how to best address
> the
> > > > UTF8
> > > > > issue. Do you have any suggestions Sebastian?
> > > >
> > > > I think you can use 'raise ValueError("")' to raise the error
> > > >
> > >
> >
>
https://hg.adblockplus.org/web.adblockplus.org/file/tip/filters/inline_file.p...
> > > >
> > > >
> > > > To keep it simple I think we can check if the file name string in the
list
> > > > contains ASCII character or not.
> > >
> > > I have no idea what issue you are talking about. Can you please comment
> inline
> > > for some context?
> >
> > Whenever static file which contains non ASCII character in the filename is
> being
> > added static content generation fails with the error:
> > "UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 13:
> > ordinal not in range(128)"
> >
> > So the thing is that the error is not being shown when we run test server
> > ("runserver.py").
> > While I asked Ben do not create PDF files which contains non ASCII
characters
> I
> > was thinking it would make also sense to implement a check to be sure that
> > whenever non developer will try to run the test server will see an error at
> > least.
> > It was a comment suggestion in the ticket, but not mandatory change.
> >
> >
> > Another solution could be to check the static filenames in "runserver.py",
if
> > you think that it make more sense I'll create a ticket for that.
>
> Yeah, the testserver should behave consistently with the page generation. So
> it's unrelated of the changes here, but should rather be addressed in the CMS.
> However, first we should figure out what the difference is that causes this
> inconsistency. (A full trace might be helpful). But let's discuss that in an
> separate issue.
Good point, created a ticket -> https://issues.adblockplus.org/ticket/3610
https://codereview.adblockplus.org/29334171/diff/29334505/filters/humandates.py File filters/humandates.py (right): https://codereview.adblockplus.org/29334171/diff/29334505/filters/humandates.... filters/humandates.py:1: On 2016/02/01 16:24:54, saroyanm wrote: > Detail: No new line needed in the beginning of the files. Done. https://codereview.adblockplus.org/29334171/diff/29334505/filters/humandates.... filters/humandates.py:4: def humandates(collection, inputFormat='%Y-%m-%d', outputFormat='%B %d, %Y'): On 2016/02/01 16:24:54, saroyanm wrote: > Not really sure if naming this function as human date really makes sense, > because you also pass the output format as parameter, so this can be changed > anytime. I think previous name was better "datesort". I changed it because it did more than sort. In the next patch I will separate the datesart and the humanize_date functionality. https://codereview.adblockplus.org/29334171/diff/29334505/filters/humandates.... filters/humandates.py:5: if inputFormat != outputFormat: On 2016/02/01 18:01:22, Sebastian Noack wrote: > This check is redundant. No need for premature micro-optimizations here. Done. https://codereview.adblockplus.org/29334171/diff/29334505/filters/humandates.... filters/humandates.py:7: item[0] = datetime.strftime(datetime.strptime(item[0], inputFormat), outputFormat) On 2016/02/01 18:01:23, Sebastian Noack wrote: > A filter changing it's input in-place is quite unexpected ...and a footgun. I agree. In the next patch I will manipulate a copy of the input. See humanize_dates. https://codereview.adblockplus.org/29334171/diff/29334505/filters/randomsortb... File filters/randomsortbydate.py (right): https://codereview.adblockplus.org/29334171/diff/29334505/filters/randomsortb... filters/randomsortbydate.py:1: On 2016/02/01 16:24:54, saroyanm wrote: > Detail: No new line needed in the beginning of the files. Done. https://codereview.adblockplus.org/29334171/diff/29334505/filters/randomsortb... filters/randomsortbydate.py:1: On 2016/02/01 18:01:23, Sebastian Noack wrote: > It seems the changes in here (except for the function name) are unrelated and > unnecessary. I'm not even sure whether they improve the code quality. Done. https://codereview.adblockplus.org/29334171/diff/29334505/includes/press/rele... File includes/press/releases.tmpl (right): https://codereview.adblockplus.org/29334171/diff/29334505/includes/press/rele... includes/press/releases.tmpl:14: ["2015-09-10", "de", "Adblock Browser aus dem Exil entlassen - endlich zurück im Google Play Store!", "2015-09-10-Adblock_Browser_für_Android-DE.pdf"], On 2016/02/01 16:24:54, saroyanm wrote: > Can you please change link to PDF file to -> > 2015-09-10-Adblock_Browser_fur_Android-DE.pdf (it's leftover from initial > implementation) Done. https://codereview.adblockplus.org/29334171/diff/29334505/includes/press/rele... includes/press/releases.tmpl:15: ["2015-09-08", "de", "Adblock Plus schneller dran als Apple - Adblock Browser kommt noch vor iOS 9!", "2015-09-08-Adblock_Browser_für_iOS_Launch-DE.pdf"], On 2016/02/01 16:24:54, saroyanm wrote: > Can you please change link to PDF file to -> > 2015-09-08-Adblock_Browser_fur_iOS_Launch-DE.pdf (it's leftover from initial > implementation) Done. https://codereview.adblockplus.org/29334171/diff/29334505/includes/press/rele... includes/press/releases.tmpl:16: ["2015-09-08", "de", "Adblock Browser aus dem Exil entlassen - endlich zurück im Google Play Store!", "2015-09-08-Adblock_Browser_für_Android-DE.pdf"], On 2016/02/01 16:24:54, saroyanm wrote: > Can you please change link to PDF file to -> > 2015-09-08-Adblock_Browser_fur_Android-DE.pdf (it's leftover from initial > implementation) Done. https://codereview.adblockplus.org/29334171/diff/29334505/includes/press/rele... includes/press/releases.tmpl:22: ] | humandates %} On 2016/02/01 18:01:23, Sebastian Noack wrote: > Nit: Please call the filter where you output the data not where you define them. > This filter is about data representation, not about data structure. Done.
https://codereview.adblockplus.org/29334171/diff/29335343/filters/humanize_da... File filters/humanize_dates.py (right): https://codereview.adblockplus.org/29334171/diff/29335343/filters/humanize_da... filters/humanize_dates.py:3: def humanize_dates (unformatted_list): Nit: No space before arguments list. https://codereview.adblockplus.org/29334171/diff/29335343/filters/humanize_da... filters/humanize_dates.py:5: for item in formatted_list: While looking at the template, is there actually any reason to perform that filter on the list, rather than on the value before it gets written to the output? That way the logic of the filter could be way simpler, if it simply process single dates instead of lists. Also it would be more reusable.
https://codereview.adblockplus.org/29334171/diff/29335343/filters/humanize_da... File filters/humanize_dates.py (right): https://codereview.adblockplus.org/29334171/diff/29335343/filters/humanize_da... filters/humanize_dates.py:3: def humanize_dates (unformatted_list): On 2016/02/02 16:50:21, Sebastian Noack wrote: > Nit: No space before arguments list. Done. https://codereview.adblockplus.org/29334171/diff/29335343/filters/humanize_da... filters/humanize_dates.py:5: for item in formatted_list: On 2016/02/02 16:50:21, Sebastian Noack wrote: > While looking at the template, is there actually any reason to perform that > filter on the list, rather than on the value before it gets written to the > output? That way the logic of the filter could be way simpler, if it simply > process single dates instead of lists. Also it would be more reusable. Done.
https://codereview.adblockplus.org/29334171/diff/29335535/filters/humanize_da... File filters/humanize_date.py (right): https://codereview.adblockplus.org/29334171/diff/29335535/filters/humanize_da... filters/humanize_date.py:1: import datetime, copy The import for the "copy" module isn't necessary anymore. https://codereview.adblockplus.org/29334171/diff/29335535/filters/humanize_da... filters/humanize_date.py:4: return datetime.datetime.strftime( Simply, call methods on the instance itself, not on the class passing the instance: datetime.datetime.strptime(date, "%Y-%m-%d").strftime("%B %d, %Y")
https://codereview.adblockplus.org/29334171/diff/29335535/filters/humanize_da... File filters/humanize_date.py (right): https://codereview.adblockplus.org/29334171/diff/29335535/filters/humanize_da... filters/humanize_date.py:1: import datetime, copy On 2016/02/04 11:05:06, Sebastian Noack wrote: > The import for the "copy" module isn't necessary anymore. Done. Good catch. https://codereview.adblockplus.org/29334171/diff/29335535/filters/humanize_da... filters/humanize_date.py:4: return datetime.datetime.strftime( On 2016/02/04 11:05:06, Sebastian Noack wrote: > Simply, call methods on the instance itself, not on the class passing the > instance: > > datetime.datetime.strptime(date, "%Y-%m-%d").strftime("%B %d, %Y") Done. I was copying the style of the previous datesort function. I will fix that function too in the next patch.
https://codereview.adblockplus.org/29334171/diff/29335550/filters/humanize_da... File filters/humanize_date.py (right): https://codereview.adblockplus.org/29334171/diff/29335550/filters/humanize_da... filters/humanize_date.py:4: return datetime.strftime( That is not what I meant. You still call the strftime method on the datetime class passing in the instance. However, you can simply call the method on the instance directly, which is not only shorter, but makes the code also easier to read as you write it in the order the actions are performed. datetime.strptime(date, "%Y-%m-%d").strftime("%B %d, %Y")
https://codereview.adblockplus.org/29334171/diff/29335550/filters/humanize_da... File filters/humanize_date.py (right): https://codereview.adblockplus.org/29334171/diff/29335550/filters/humanize_da... filters/humanize_date.py:4: return datetime.strftime( On 2016/02/04 11:31:42, Sebastian Noack wrote: > That is not what I meant. You still call the strftime method on the datetime > class passing in the instance. However, you can simply call the method on the > instance directly, which is not only shorter, but makes the code also easier to > read as you write it in the order the actions are performed. > > datetime.strptime(date, "%Y-%m-%d").strftime("%B %d, %Y") Done.
LGTM
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
