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

Issue 29555839: Issue 5336 - Allow additional include, page, and template paths using CMS (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 8 months ago by Vasily Kuznetsov
Modified:
1 year, 7 months ago
CC:
wspee
Visibility:
Public.

Description

Issue 5336 - Allow additional include, page, and template paths using CMS repository: https://hg.adblockplus.org/cms base revision: fb78657d24c3

Patch Set 1 #

Total comments: 9

Patch Set 2 : Address comments on PS1 #

Patch Set 3 : Address comments on PS2, improve docstring of create_source #

Total comments: 10

Patch Set 4 : Address comments on PS3 #

Total comments: 2

Patch Set 5 : Rename 'static' parameter to 'cached' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+395 lines, -49 lines) Patch
M cms/bin/generate_static_pages.py View 1 2 3 4 3 chunks +3 lines, -24 lines 0 comments Download
M cms/bin/test_server.py View 2 chunks +2 lines, -2 lines 0 comments Download
M cms/sources.py View 1 2 3 4 1 chunk +106 lines, -0 lines 0 comments Download
A tests/__init__.py View 0 chunks +-1 lines, --1 lines 0 comments Download
A tests/test_additional_paths.py View 1 2 3 1 chunk +110 lines, -0 lines 0 comments Download
A tests/test_multi_source.py View 1 chunk +121 lines, -0 lines 0 comments Download
M tests/test_page_outputs.py View 2 chunks +5 lines, -24 lines 0 comments Download
A tests/utils.py View 1 chunk +49 lines, -0 lines 0 comments Download

Messages

Total messages: 19
Vasily Kuznetsov
Hi Jon, Sebastian and Julian! Here's the implementation of the "additional paths" feature. I created ...
1 year, 8 months ago (2017-09-26 13:26:47 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29555839/diff/29555840/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29555839/diff/29555840/cms/sources.py#newcode335 cms/sources.py:335: for base in self._bases: Nit: You could use any() ...
1 year, 8 months ago (2017-09-27 02:47:53 UTC) #2
Vasily Kuznetsov
Hi Sebastian, Thank you for the comments. I've implemented the first two and replied to ...
1 year, 8 months ago (2017-09-27 11:34:39 UTC) #3
Sebastian Noack
https://codereview.adblockplus.org/29555839/diff/29555840/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29555839/diff/29555840/cms/sources.py#newcode350 cms/sources.py:350: return sorted(files) On 2017/09/27 11:34:39, Vasily Kuznetsov wrote: > ...
1 year, 8 months ago (2017-09-27 15:55:22 UTC) #4
Vasily Kuznetsov
I removed the conversion of set to list in `MultiSource.list_files` following Sebastian's suggestion and improved ...
1 year, 8 months ago (2017-09-28 09:08:17 UTC) #5
Sebastian Noack
https://codereview.adblockplus.org/29555839/diff/29558570/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29555839/diff/29558570/cms/sources.py#newcode396 cms/sources.py:396: create_source(os.path.join(path, p)) This seems to be equivalent to FileSource(os.path.join(path, ...
1 year, 8 months ago (2017-09-28 20:48:21 UTC) #6
Wladimir Palant
https://codereview.adblockplus.org/29555839/diff/29558570/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29555839/diff/29558570/cms/sources.py#newcode360 cms/sources.py:360: def create_source(path, static=False, revision='default'): The default revision should be ...
1 year, 7 months ago (2017-09-29 08:50:44 UTC) #7
Vasily Kuznetsov
Hi Wladimir, Thank you for the comments. I have addressed all except for the last ...
1 year, 7 months ago (2017-09-29 10:49:10 UTC) #8
juliandoucette
> Could you please comment on your preferences as to whether the CMS should rely ...
1 year, 7 months ago (2017-09-29 13:34:05 UTC) #9
Sebastian Noack
https://codereview.adblockplus.org/29555839/diff/29558570/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29555839/diff/29558570/cms/sources.py#newcode396 cms/sources.py:396: create_source(os.path.join(path, p)) On 2017/09/29 10:49:09, Vasily Kuznetsov wrote: > ...
1 year, 7 months ago (2017-09-29 15:55:08 UTC) #10
Vasily Kuznetsov
On 2017/09/29 15:55:08, Sebastian Noack wrote: > https://codereview.adblockplus.org/29555839/diff/29558570/cms/sources.py > File cms/sources.py (right): > > https://codereview.adblockplus.org/29555839/diff/29558570/cms/sources.py#newcode396 ...
1 year, 7 months ago (2017-09-29 17:28:53 UTC) #11
Wladimir Palant
Patch set 4 looks fine. As to managing dependencies, it's a bit questionable whether npm ...
1 year, 7 months ago (2017-09-29 18:08:28 UTC) #12
Sebastian Noack
I still consider this approach rather questionable. npm doesn't seem to be the right tool ...
1 year, 7 months ago (2017-09-29 19:11:20 UTC) #13
Sebastian Noack
https://codereview.adblockplus.org/29555839/diff/29559578/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29555839/diff/29559578/cms/sources.py#newcode360 cms/sources.py:360: def create_source(path, static=False, revision=None): Does it still make sense ...
1 year, 7 months ago (2017-09-29 19:11:30 UTC) #14
Vasily Kuznetsov
On 2017/09/29 19:11:20, Sebastian Noack wrote: > I still consider this approach rather questionable. npm ...
1 year, 7 months ago (2017-10-02 11:11:56 UTC) #15
Jon Sonesen
On 2017/09/29 19:11:20, Sebastian Noack wrote: > I still consider this approach rather questionable. npm ...
1 year, 7 months ago (2017-10-02 18:52:01 UTC) #16
Vasily Kuznetsov
On 2017/10/02 18:52:01, Jon Sonesen wrote: > On 2017/09/29 19:11:20, Sebastian Noack wrote: > > ...
1 year, 7 months ago (2017-10-02 19:50:37 UTC) #17
Sebastian Noack
On 2017/10/02 11:11:56, Vasily Kuznetsov wrote: > On 2017/09/29 19:11:20, Sebastian Noack wrote: > > ...
1 year, 7 months ago (2017-10-02 23:51:42 UTC) #18
Jon Sonesen
1 year, 7 months ago (2017-10-03 19:51:12 UTC) #19
> 1. Based on all the comments above, do you think that the text of #5336 should
> be changed?

No, I guess it is fine how it is since the comments point out the split of
responsibilities or "scope" for the cms as well as the future need to
uncomplicate that nuance of the cms. Although, I will add that this will
increase the need for documentation that is available for all developers not
only for the cms but likely for the websites so that (as sebastian pointed out)
we will be able to make relatively painless changes to websites if we need to.

> 3. If `answer[1] is False`, does the implementation here have any problems
that
> I should fix?

No, I agree that the implementation is functional, so LGTM
Sign in to reply to this message.

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