Issue 29328521: Issue 133 - Add README to sitescripts repository (Closed)
|
Created:
Sept. 23, 2015, 10:23 a.m. by kzar Modified:
Oct. 8, 2015, 12:12 p.m. CC:
Felix Dahlke Visibility:
Public. |
DescriptionIssue 133 - Add README to sitescripts repository
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed feedback #
Total comments: 4
Patch Set 3 : Addressed some final nits #MessagesTotal messages: 8
Patch Set 1
https://codereview.adblockplus.org/29328521/diff/29328522/README.md File README.md (right): https://codereview.adblockplus.org/29328521/diff/29328522/README.md#newcode25 README.md:25: 1.) ~/.sitescripts Nit: Using closing parenthesis for numbered lists in markdown looks weird to me. I just tested it, and in fact, this syntax isn't recognized by GitHub. https://codereview.adblockplus.org/29328521/diff/29328522/README.md#newcode27 README.md:27: 3.) /etc/sitescripts, I guess the trailing comma is a typo? https://codereview.adblockplus.org/29328521/diff/29328522/README.md#newcode50 README.md:50: These scripts are written in a standard way, using some of the decorators and I think it makes sense to explicitly refer to WSGI and/or PEP-333 as the standard here.
https://codereview.adblockplus.org/29328521/diff/29328522/README.md File README.md (right): https://codereview.adblockplus.org/29328521/diff/29328522/README.md#newcode23 README.md:23: The configuration file can be placed in a number of locations: Not just that, the code will actually look for it in that specific order. So if ~/.sitescripts exists then it shadows all the other locations. https://codereview.adblockplus.org/29328521/diff/29328522/README.md#newcode31 README.md:31: provide a custom path for the configuration._ Please note explicitly that it will override the default configuration paths - so the individual configuration files aren't being merged or anything like that. https://codereview.adblockplus.org/29328521/diff/29328522/README.md#newcode57 README.md:57: `multiplexer.py` in development. Probably mention explicitly that multiplexer.py is a stand-alone server whereas multiplexer.fcgi is merely an FCGI script? Also, the latter requires flup whereas the latter doesn't have any dependencies but installing werkzeug will add debugging tools.
Patch Set 2 : Addressed feedback https://codereview.adblockplus.org/29328521/diff/29328522/README.md File README.md (right): https://codereview.adblockplus.org/29328521/diff/29328522/README.md#newcode23 README.md:23: The configuration file can be placed in a number of locations: On 2015/10/05 11:22:20, Wladimir Palant wrote: > Not just that, the code will actually look for it in that specific order. So if > ~/.sitescripts exists then it shadows all the other locations. Done. https://codereview.adblockplus.org/29328521/diff/29328522/README.md#newcode25 README.md:25: 1.) ~/.sitescripts On 2015/10/05 10:56:57, Sebastian Noack wrote: > Nit: Using closing parenthesis for numbered lists in markdown looks weird to me. > I just tested it, and in fact, this syntax isn't recognized by GitHub. Done. https://codereview.adblockplus.org/29328521/diff/29328522/README.md#newcode27 README.md:27: 3.) /etc/sitescripts, On 2015/10/05 10:56:57, Sebastian Noack wrote: > I guess the trailing comma is a typo? Done. https://codereview.adblockplus.org/29328521/diff/29328522/README.md#newcode31 README.md:31: provide a custom path for the configuration._ On 2015/10/05 11:22:20, Wladimir Palant wrote: > Please note explicitly that it will override the default configuration paths - > so the individual configuration files aren't being merged or anything like that. Done. https://codereview.adblockplus.org/29328521/diff/29328522/README.md#newcode50 README.md:50: These scripts are written in a standard way, using some of the decorators and On 2015/10/05 10:56:57, Sebastian Noack wrote: > I think it makes sense to explicitly refer to WSGI and/or PEP-333 as the > standard here. Done. https://codereview.adblockplus.org/29328521/diff/29328522/README.md#newcode57 README.md:57: `multiplexer.py` in development. On 2015/10/05 11:22:20, Wladimir Palant wrote: > Probably mention explicitly that multiplexer.py is a stand-alone server whereas > multiplexer.fcgi is merely an FCGI script? Also, the latter requires flup > whereas the latter doesn't have any dependencies but installing werkzeug will > add debugging tools. Done.
https://codereview.adblockplus.org/29328521/diff/29328861/README.md File README.md (right): https://codereview.adblockplus.org/29328521/diff/29328861/README.md#newcode74 README.md:74: So, to test any of the URL handlers in development simply do the following: Nit: too many "simply" - this usually indicates that things aren't simple after all. Please consider dropping it on both occasions. https://codereview.adblockplus.org/29328521/diff/29328861/README.md#newcode82 README.md:82: 5000. This web server will use any URL handlers that have been defined in the "locally on port 5000" => "at http://localhost:5000/"?
Patch Set 3 : Addressed some final nits https://codereview.adblockplus.org/29328521/diff/29328861/README.md File README.md (right): https://codereview.adblockplus.org/29328521/diff/29328861/README.md#newcode74 README.md:74: So, to test any of the URL handlers in development simply do the following: On 2015/10/06 18:39:36, Wladimir Palant wrote: > Nit: too many "simply" - this usually indicates that things aren't simple after > all. Please consider dropping it on both occasions. Done. https://codereview.adblockplus.org/29328521/diff/29328861/README.md#newcode82 README.md:82: 5000. This web server will use any URL handlers that have been defined in the On 2015/10/06 18:39:36, Wladimir Palant wrote: > "locally on port 5000" => "at http://localhost:5000/%22 Done.
LGTM
LGTM 
|
