Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29516687: Issue 4488 - Add support for JSON page front matter (Closed)

Created:
Aug. 16, 2017, 12:43 a.m. by rosie
Modified:
May 1, 2018, 6:45 p.m.
Base URL:
https://hg.adblockplus.org/cms
Visibility:
Public.

Description

Issue 4488 - Add support for JSON page front matter

Patch Set 1 #

Total comments: 12

Patch Set 2 : Removed JSON postprocessing and integrated the cms tests #

Total comments: 6

Patch Set 3 : Cleaned up duplication, removed unnecessary regex #

Total comments: 18

Patch Set 4 : Preserve lines in metadata, fix sort in sitemap.tmpl #

Total comments: 14

Patch Set 5 : Add support for pages with no metadata #

Total comments: 6

Patch Set 6 : Fixed .gitignore and comment placement #

Patch Set 7 : Test cases no longer use 'template = empty' #

Total comments: 6

Patch Set 8 : Add newline for code clarity #

Patch Set 9 : Removed the sitemap sort attribute #

Unified diffs Side-by-side diffs Delta from patch set Stats (+106 lines, -16 lines) Patch
M cms/utils.py View 1 2 3 4 5 6 7 2 chunks +29 lines, -15 lines 0 comments Download
A tests/expected_output/en/metadata_json View 1 2 3 4 5 6 1 chunk +13 lines, -0 lines 0 comments Download
A tests/expected_output/en/metadata_json_comment View 1 2 3 4 5 6 1 chunk +15 lines, -0 lines 0 comments Download
A tests/expected_output/en/metadata_old_comment View 1 2 3 4 5 6 1 chunk +9 lines, -0 lines 0 comments Download
M tests/expected_output/en/sitemap View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -1 line 0 comments Download
A tests/test_site/pages/metadata_json.tmpl View 1 2 3 4 5 6 1 chunk +13 lines, -0 lines 0 comments Download
A tests/test_site/pages/metadata_json_comment.tmpl View 1 2 3 4 5 6 1 chunk +15 lines, -0 lines 0 comments Download
A tests/test_site/pages/metadata_old_comment.tmpl View 1 2 3 4 5 6 1 chunk +9 lines, -0 lines 0 comments Download

Messages

Total messages: 29
rosie
Aug. 16, 2017, 12:43 a.m. (2017-08-16 00:43:53 UTC) #1
Vasily Kuznetsov
Hi Rosie! I think your approach in `converters.py` goes in the right direction but needs ...
Aug. 16, 2017, 6:34 p.m. (2017-08-16 18:34:57 UTC) #2
rosie
Hope you guys have a great weekend! :) https://codereview.adblockplus.org/29516687/diff/29516688/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29516687/diff/29516688/cms/converters.py#newcode122 cms/converters.py:122: def ...
Aug. 19, 2017, 1:56 a.m. (2017-08-19 01:56:35 UTC) #3
Vasily Kuznetsov
Hi Rosie, This looks pretty good! I have no comments on the tests, all is ...
Aug. 21, 2017, 6:19 p.m. (2017-08-21 18:19:51 UTC) #4
rosie
https://codereview.adblockplus.org/29516687/diff/29519680/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29516687/diff/29519680/cms/converters.py#newcode122 cms/converters.py:122: def parse_metadata(page, data): On 2017/08/21 18:19:50, Vasily Kuznetsov wrote: ...
Aug. 23, 2017, 6:13 p.m. (2017-08-23 18:13:05 UTC) #5
Vasily Kuznetsov
LGTM https://codereview.adblockplus.org/29516687/diff/29519680/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29516687/diff/29519680/cms/converters.py#newcode148 cms/converters.py:148: start_index = re.search('<!--', data).end() On 2017/08/23 18:13:05, rosie ...
Aug. 25, 2017, 10:12 a.m. (2017-08-25 10:12:28 UTC) #6
Jon Sonesen
On 2017/08/25 10:12:28, Vasily Kuznetsov wrote: > LGTM > > https://codereview.adblockplus.org/29516687/diff/29519680/cms/converters.py > File cms/converters.py (right): ...
Aug. 28, 2017, 11:33 a.m. (2017-08-28 11:33:07 UTC) #7
Sebastian Noack
https://codereview.adblockplus.org/29516687/diff/29525608/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29516687/diff/29525608/cms/converters.py#newcode38 cms/converters.py:38: Adding this blank line is unrelated. https://codereview.adblockplus.org/29516687/diff/29525608/cms/converters.py#newcode128 cms/converters.py:128: page_data ...
Aug. 29, 2017, 10:49 p.m. (2017-08-29 22:49:40 UTC) #8
Vasily Kuznetsov
Thanks for those points, Sebastian! Rosie, there are few more things to consider if we ...
Aug. 30, 2017, 10:50 a.m. (2017-08-30 10:50:01 UTC) #9
rosie
Hey guys! :) It seems this functionality was moved to utils.py, so most of the ...
March 26, 2018, 2:32 a.m. (2018-03-26 02:32:22 UTC) #10
Sebastian Noack
https://codereview.adblockplus.org/29516687/diff/29525608/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29516687/diff/29525608/cms/converters.py#newcode38 cms/converters.py:38: On 2018/03/26 02:32:21, rosie wrote: > On 2017/08/29 22:49:39, ...
March 26, 2018, 2:57 a.m. (2018-03-26 02:57:48 UTC) #11
Vasily Kuznetsov
Hi Rosie, I don't have much to add to Sebastian's comments, just a couple of ...
March 26, 2018, 9:24 a.m. (2018-03-26 09:24:29 UTC) #12
Jon Sonesen
https://codereview.adblockplus.org/29516687/diff/29732555/cms/utils.py File cms/utils.py (right): https://codereview.adblockplus.org/29516687/diff/29732555/cms/utils.py#newcode93 cms/utils.py:93: return metadata, '\n' * source.count('\n', 0, cutoff) + source[cutoff:] ...
March 28, 2018, 3:01 a.m. (2018-03-28 03:01:37 UTC) #13
rosie
Thanks for the feedback! I implemented the change, but ran into some trouble making the ...
March 31, 2018, 9:09 p.m. (2018-03-31 21:09:28 UTC) #14
Sebastian Noack
https://codereview.adblockplus.org/29516687/diff/29732555/cms/utils.py File cms/utils.py (right): https://codereview.adblockplus.org/29516687/diff/29732555/cms/utils.py#newcode93 cms/utils.py:93: return metadata, '\n' * source.count('\n', 0, cutoff) + source[cutoff:] ...
March 31, 2018, 10:07 p.m. (2018-03-31 22:07:36 UTC) #15
Vasily Kuznetsov
Hi Rosie, Sorry about the roadblocks, fixing them! Cheers, Vasily https://codereview.adblockplus.org/29516687/diff/29732555/cms/utils.py File cms/utils.py (right): https://codereview.adblockplus.org/29516687/diff/29732555/cms/utils.py#newcode93 ...
April 3, 2018, 2:29 p.m. (2018-04-03 14:29:20 UTC) #16
Jon Sonesen
On 2018/03/31 22:07:36, Sebastian Noack wrote: > https://codereview.adblockplus.org/29516687/diff/29732555/cms/utils.py > File cms/utils.py (right): > > https://codereview.adblockplus.org/29516687/diff/29732555/cms/utils.py#newcode93 ...
April 3, 2018, 3:48 p.m. (2018-04-03 15:48:02 UTC) #17
Vasily Kuznetsov
On 2018/04/03 15:48:02, Jon Sonesen wrote: > On 2018/03/31 22:07:36, Sebastian Noack wrote: > > ...
April 3, 2018, 4:25 p.m. (2018-04-03 16:25:12 UTC) #18
rosie
Please ignore this patch-set! I just noticed that something's wrong with the test cases... I ...
April 18, 2018, 2:35 a.m. (2018-04-18 02:35:33 UTC) #19
Sebastian Noack
https://codereview.adblockplus.org/29516687/diff/29738607/.hgignore File .hgignore (right): https://codereview.adblockplus.org/29516687/diff/29738607/.hgignore#newcode11 .hgignore:11: .pytest_cache On 2018/04/18 02:35:32, rosie wrote: > On 2018/03/31 ...
April 18, 2018, 5:01 a.m. (2018-04-18 05:01:40 UTC) #20
rosie
I figured out the issue. My test cases were still using 'template = empty' in ...
April 18, 2018, 8:47 p.m. (2018-04-18 20:47:45 UTC) #21
Sebastian Noack
Basically LGTM. I don't care if the remaining nits get addressed. They are quite unimportant. ...
April 18, 2018, 9:12 p.m. (2018-04-18 21:12:28 UTC) #22
Sebastian Noack
Basically LGTM. I don't care if the remaining nits get addressed. They are quite unimportant.
April 18, 2018, 9:12 p.m. (2018-04-18 21:12:30 UTC) #23
rosie
Added the suggested newline, and had a question about the changes in sitescripts.tmpl https://codereview.adblockplus.org/29516687/diff/29755796/cms/utils.py File ...
April 18, 2018, 10:46 p.m. (2018-04-18 22:46:19 UTC) #24
Sebastian Noack
https://codereview.adblockplus.org/29516687/diff/29755796/tests/test_site/pages/sitemap.tmpl File tests/test_site/pages/sitemap.tmpl (right): https://codereview.adblockplus.org/29516687/diff/29755796/tests/test_site/pages/sitemap.tmpl#newcode6 tests/test_site/pages/sitemap.tmpl:6: {%- for unfiltered_page in get_pages_metadata()|sort(attribute='page') %} On 2018/04/18 22:46:18, ...
April 19, 2018, 12:22 a.m. (2018-04-19 00:22:25 UTC) #25
rosie
Removed the sitemap sort attribute. I'll submit it as a noissue shortly. https://codereview.adblockplus.org/29516687/diff/29755796/tests/test_site/pages/sitemap.tmpl File tests/test_site/pages/sitemap.tmpl ...
April 19, 2018, 3:31 a.m. (2018-04-19 03:31:02 UTC) #26
Sebastian Noack
LGTM
April 19, 2018, 7:05 a.m. (2018-04-19 07:05:20 UTC) #27
Sebastian Noack
LGTM
April 19, 2018, 7:05 a.m. (2018-04-19 07:05:23 UTC) #28
Vasily Kuznetsov
April 19, 2018, 3:29 p.m. (2018-04-19 15:29:48 UTC) #29
LGTM

Powered by Google App Engine
This is Rietveld