Issue 29334114: issue 3546 - Add port and hostname options to CMS testing server (Closed)
|
Created:
Jan. 20, 2016, 1:39 p.m. by juliandoucette Modified:
Jan. 25, 2016, 2:21 p.m. Reviewers:
Sebastian Noack CC:
kzar, saroyanm Visibility:
Public. |
Descriptionissue 3546 - Add port and hostname options to CMS testing server
I added options using argparse, falling back to the old format if the first argument is a directory and not an option.
We may want to add additional documentation or find a way to support `runserver -p 8080 /path/to/base/` style too.
Patch Set 1 #
Total comments: 8
Patch Set 2 : Changed basepath to positional argument and added defaults to hostname and port #
Total comments: 10
Patch Set 3 : changed defaults, variable names, and updated get_page #Patch Set 4 : See help message not necessary #
Total comments: 12
Patch Set 5 : Removed listen argument in favor of address and port, updated help text, and updated formatted stri… #
Total comments: 5
Patch Set 6 : Fixes help text wording #MessagesTotal messages: 15
https://codereview.adblockplus.org/29334114/diff/29334115/cms/bin/test_server.py File cms/bin/test_server.py (right): https://codereview.adblockplus.org/29334114/diff/29334115/cms/bin/test_server... cms/bin/test_server.py:122: parser.add_argument('-b', '--basepath', type=str, help='Base directory (EG: /path/to/web.eyeo.com)') Please keep the path a positional argument rather than a named option, for convenience and compatibility. And make it default to os.curdir. https://codereview.adblockplus.org/29334114/diff/29334115/cms/bin/test_server... cms/bin/test_server.py:123: parser.add_argument('-n', '--hostname', type=str, help='Server hostname (EG: localhost, eyeo.com, 0.0.0.0)') Specify "localhost" as default. https://codereview.adblockplus.org/29334114/diff/29334115/cms/bin/test_server... cms/bin/test_server.py:124: parser.add_argument('-p', '--port', type=str, help='Port number (EG: 8080, 5000)') Specify 5000 as default. https://codereview.adblockplus.org/29334114/diff/29334115/cms/bin/test_server... cms/bin/test_server.py:127: # for backwards compatibility With the above comments addressed you don't need to switch the logic here.
On 2016/01/20 14:35:59, Sebastian Noack wrote: > https://codereview.adblockplus.org/29334114/diff/29334115/cms/bin/test_server.py > File cms/bin/test_server.py (right): > > https://codereview.adblockplus.org/29334114/diff/29334115/cms/bin/test_server... > cms/bin/test_server.py:122: parser.add_argument('-b', '--basepath', type=str, > help='Base directory (EG: /path/to/web.eyeo.com)') > Please keep the path a positional argument rather than a named option, for > convenience and compatibility. And make it default to os.curdir. > > https://codereview.adblockplus.org/29334114/diff/29334115/cms/bin/test_server... > cms/bin/test_server.py:123: parser.add_argument('-n', '--hostname', type=str, > help='Server hostname (EG: localhost, http://eyeo.com, 0.0.0.0)') > Specify "localhost" as default. > > https://codereview.adblockplus.org/29334114/diff/29334115/cms/bin/test_server... > cms/bin/test_server.py:124: parser.add_argument('-p', '--port', type=str, > help='Port number (EG: 8080, 5000)') > Specify 5000 as default. > > https://codereview.adblockplus.org/29334114/diff/29334115/cms/bin/test_server... > cms/bin/test_server.py:127: # for backwards compatibility > With the above comments addressed you don't need to switch the logic here. Thanks, these issues should be addressed in patch set 2 :) .
Also mind the get_page function at the top of the file? The port number there needs to be adapted too. Also mind uploading full patches when addressing comments, and not only the changes to your previous version? https://codereview.adblockplus.org/29334114/diff/29334177/cms/bin/test_server.py File cms/bin/test_server.py (right): https://codereview.adblockplus.org/29334114/diff/29334177/cms/bin/test_server... cms/bin/test_server.py:122: parser.add_argument('basepath', nargs='?', default=os.curdir) Nit: I would simply call it "path". https://codereview.adblockplus.org/29334114/diff/29334177/cms/bin/test_server... cms/bin/test_server.py:123: parser.add_argument('-n', '--hostname', default='localhost', type=str, help='Server hostname (EG: localhost, example.com, 0.0.0.0)') type=str is the default. How about omitting it, if it's just for consistence with the line above? https://codereview.adblockplus.org/29334114/diff/29334177/cms/bin/test_server... cms/bin/test_server.py:123: parser.add_argument('-n', '--hostname', default='localhost', type=str, help='Server hostname (EG: localhost, example.com, 0.0.0.0)') Nit: "Address of the interface the server will listen on" would be a more accurate help text. Also the examples aren't probably necessary. https://codereview.adblockplus.org/29334114/diff/29334177/cms/bin/test_server... cms/bin/test_server.py:124: parser.add_argument('-p', '--port', default='5000', type=str, help='Port number (EG: 8080, 5000)') How about type=int? Then you don't need to convert the value below.
https://codereview.adblockplus.org/29334114/diff/29334177/cms/bin/test_server.py File cms/bin/test_server.py (right): https://codereview.adblockplus.org/29334114/diff/29334177/cms/bin/test_server... cms/bin/test_server.py:123: parser.add_argument('-n', '--hostname', default='localhost', type=str, help='Server hostname (EG: localhost, example.com, 0.0.0.0)') --address would be a better name for the option, as this isn't a hostname but the address of the interface. While this can be the same, it's not necessarily true, e.g. 0.0.0.0 is never a hostname but an address that refers to all interfaces. Alternatively, you could add an option --listen [address]:port instead --address and --port. This would be a little quicker to type. And it would be somewhat consistent with django's runserver command. But I wouldn't insist.
> Also mind uploading full patches when addressing comments, and not only the > changes to your previous version? To the codereview or Trac?
On 2016/01/21 09:13:54, juliandoucette wrote: > > Also mind uploading full patches when addressing comments, and not only the > > changes to your previous version? > > To the codereview or Trac? I'm talking about Rietveld, this web app which we use for code review. When I click on the second patch you uploaded above, I only see the changes to the first patch, not to the current code in the repo. That's probably because you created a new commit in your local repo (rather than using --amend) and only uploaded the diff for that second commit.
Thanks :) https://codereview.adblockplus.org/29334114/diff/29334115/cms/bin/test_server.py File cms/bin/test_server.py (right): https://codereview.adblockplus.org/29334114/diff/29334115/cms/bin/test_server... cms/bin/test_server.py:122: parser.add_argument('-b', '--basepath', type=str, help='Base directory (EG: /path/to/web.eyeo.com)') On 2016/01/20 14:35:59, Sebastian Noack wrote: > Please keep the path a positional argument rather than a named option, for > convenience and compatibility. And make it default to os.curdir. Done. https://codereview.adblockplus.org/29334114/diff/29334115/cms/bin/test_server... cms/bin/test_server.py:123: parser.add_argument('-n', '--hostname', type=str, help='Server hostname (EG: localhost, eyeo.com, 0.0.0.0)') On 2016/01/20 14:35:59, Sebastian Noack wrote: > Specify "localhost" as default. Done. https://codereview.adblockplus.org/29334114/diff/29334115/cms/bin/test_server... cms/bin/test_server.py:124: parser.add_argument('-p', '--port', type=str, help='Port number (EG: 8080, 5000)') On 2016/01/20 14:35:59, Sebastian Noack wrote: > Specify 5000 as default. Done. https://codereview.adblockplus.org/29334114/diff/29334115/cms/bin/test_server... cms/bin/test_server.py:127: # for backwards compatibility On 2016/01/20 14:35:59, Sebastian Noack wrote: > With the above comments addressed you don't need to switch the logic here. Agreed. Thank you for bringing that up. https://codereview.adblockplus.org/29334114/diff/29334177/cms/bin/test_server.py File cms/bin/test_server.py (right): https://codereview.adblockplus.org/29334114/diff/29334177/cms/bin/test_server... cms/bin/test_server.py:122: parser.add_argument('basepath', nargs='?', default=os.curdir) On 2016/01/20 17:46:25, Sebastian Noack wrote: > Nit: I would simply call it "path". Done. https://codereview.adblockplus.org/29334114/diff/29334177/cms/bin/test_server... cms/bin/test_server.py:123: parser.add_argument('-n', '--hostname', default='localhost', type=str, help='Server hostname (EG: localhost, example.com, 0.0.0.0)') On 2016/01/20 17:46:25, Sebastian Noack wrote: > Nit: "Address of the interface the server will listen on" would be a more > accurate help text. Also the examples aren't probably necessary. Done. https://codereview.adblockplus.org/29334114/diff/29334177/cms/bin/test_server... cms/bin/test_server.py:123: parser.add_argument('-n', '--hostname', default='localhost', type=str, help='Server hostname (EG: localhost, example.com, 0.0.0.0)') On 2016/01/20 17:46:25, Sebastian Noack wrote: > type=str is the default. How about omitting it, if it's just for consistence > with the line above? Done. https://codereview.adblockplus.org/29334114/diff/29334177/cms/bin/test_server... cms/bin/test_server.py:123: parser.add_argument('-n', '--hostname', default='localhost', type=str, help='Server hostname (EG: localhost, example.com, 0.0.0.0)') On 2016/01/20 18:00:55, Sebastian Noack wrote: > --address would be a better name for the option, as this isn't a hostname but > the address of the interface. While this can be the same, it's not necessarily > true, e.g. 0.0.0.0 is never a hostname but an address that refers to all > interfaces. > > Alternatively, you could add an option --listen [address]:port instead --address > and --port. This would be a little quicker to type. And it would be somewhat > consistent with django's runserver command. But I wouldn't insist. Acknowledged. https://codereview.adblockplus.org/29334114/diff/29334177/cms/bin/test_server... cms/bin/test_server.py:124: parser.add_argument('-p', '--port', default='5000', type=str, help='Port number (EG: 8080, 5000)') On 2016/01/20 17:46:25, Sebastian Noack wrote: > How about type=int? Then you don't need to convert the value below. Done.
https://codereview.adblockplus.org/29334114/diff/29334184/cms/bin/test_server.py File cms/bin/test_server.py (right): https://codereview.adblockplus.org/29334114/diff/29334184/cms/bin/test_server... cms/bin/test_server.py:30: address = 'localhost' Please don't define defaults redundantly. You can simply set address and port to None here. It will get overridden when the arguments have been parsed anyway. https://codereview.adblockplus.org/29334114/diff/29334184/cms/bin/test_server... cms/bin/test_server.py:71: return (p, process_page(source, locale, p, format, "http://" + address + ":" + str(port))) Nit: Please use format strings when concatenating more than one string: "http://%s:%d" % (address, port) https://codereview.adblockplus.org/29334114/diff/29334184/cms/bin/test_server... cms/bin/test_server.py:125: parser.add_argument('-n', '--address', default='localhost', help='Address of the interface the server will listen to.') Nit: It's semantically not a full sentence. So no full stop. Nit: I think it should be "listen on". https://codereview.adblockplus.org/29334114/diff/29334184/cms/bin/test_server... cms/bin/test_server.py:126: parser.add_argument('-p', '--port', type=int, default=5000, help='Port number.') Nit: This help text doesn't tell anything that the option wouldn't already tell by it's name. Better: "The TCP port the server will listen on". https://codereview.adblockplus.org/29334114/diff/29334184/cms/bin/test_server... cms/bin/test_server.py:127: parser.add_argument('-l', '--listen', help='Address and port number separated by a semicolin.') I didn't meant to implement both, --address/--port and --listen, but only either of them. It's usually not a good thing to provide two ways to achieve the same thing. And the added complexity to support both isn't certainly worth it. I leave it up to you which one you will go with. https://codereview.adblockplus.org/29334114/diff/29334184/cms/bin/test_server... cms/bin/test_server.py:135: listen = str.split(args.listen, ':') Nit: args.listen.split(":")
Thanks :) https://codereview.adblockplus.org/29334114/diff/29334184/cms/bin/test_server.py File cms/bin/test_server.py (right): https://codereview.adblockplus.org/29334114/diff/29334184/cms/bin/test_server... cms/bin/test_server.py:30: address = 'localhost' On 2016/01/21 14:14:09, Sebastian Noack wrote: > Please don't define defaults redundantly. You can simply set address and port to > None here. It will get overridden when the arguments have been parsed anyway. Acknowledged. https://codereview.adblockplus.org/29334114/diff/29334184/cms/bin/test_server... cms/bin/test_server.py:71: return (p, process_page(source, locale, p, format, "http://" + address + ":" + str(port))) On 2016/01/21 14:14:08, Sebastian Noack wrote: > Nit: Please use format strings when concatenating more than one string: > > "http://%s:%d" % (address, port) Acknowledged. https://codereview.adblockplus.org/29334114/diff/29334184/cms/bin/test_server... cms/bin/test_server.py:125: parser.add_argument('-n', '--address', default='localhost', help='Address of the interface the server will listen to.') On 2016/01/21 14:14:08, Sebastian Noack wrote: > Nit: It's semantically not a full sentence. So no full stop. > Nit: I think it should be "listen on". Acknowledged. https://codereview.adblockplus.org/29334114/diff/29334184/cms/bin/test_server... cms/bin/test_server.py:126: parser.add_argument('-p', '--port', type=int, default=5000, help='Port number.') On 2016/01/21 14:14:09, Sebastian Noack wrote: > Nit: This help text doesn't tell anything that the option wouldn't already tell > by it's name. Better: "The TCP port the server will listen on". Acknowledged. https://codereview.adblockplus.org/29334114/diff/29334184/cms/bin/test_server... cms/bin/test_server.py:127: parser.add_argument('-l', '--listen', help='Address and port number separated by a semicolin.') On 2016/01/21 14:14:08, Sebastian Noack wrote: > I didn't meant to implement both, --address/--port and --listen, but only either > of them. It's usually not a good thing to provide two ways to achieve the same > thing. And the added complexity to support both isn't certainly worth it. I > leave it up to you which one you will go with. - Sorry, I misunderstood - I agree - I will go with --address/--port because it is more straightforward https://codereview.adblockplus.org/29334114/diff/29334184/cms/bin/test_server... cms/bin/test_server.py:135: listen = str.split(args.listen, ':') On 2016/01/21 14:14:08, Sebastian Noack wrote: > Nit: args.listen.split(":") Acknowledged.
https://codereview.adblockplus.org/29334114/diff/29334186/cms/bin/test_server.py File cms/bin/test_server.py (right): https://codereview.adblockplus.org/29334114/diff/29334186/cms/bin/test_server... cms/bin/test_server.py:123: parser = argparse.ArgumentParser(description='CMS testing server.') Nit: As below, no full stop if it isn't a sentence. Also, this help text could be as well omitted or feel free to add some useful information: "CMS development server to test pages locally and on the-fly" https://codereview.adblockplus.org/29334114/diff/29334186/cms/bin/test_server... cms/bin/test_server.py:125: parser.add_argument('-a', '--address', default='localhost', help='Address of the interface the server will listen on') Nit: Mind making the help messages consistent? Either add a definite article here or remove it below.
https://codereview.adblockplus.org/29334114/diff/29334186/cms/bin/test_server.py File cms/bin/test_server.py (right): https://codereview.adblockplus.org/29334114/diff/29334186/cms/bin/test_server... cms/bin/test_server.py:123: parser = argparse.ArgumentParser(description='CMS testing server.') On 2016/01/21 15:12:32, Sebastian Noack wrote: > Nit: As below, no full stop if it isn't a sentence. Also, this help text could > be as well omitted or feel free to add some useful information: "CMS development > server to test pages locally and on the-fly" Done. https://codereview.adblockplus.org/29334114/diff/29334186/cms/bin/test_server... cms/bin/test_server.py:125: parser.add_argument('-a', '--address', default='localhost', help='Address of the interface the server will listen on') On 2016/01/21 15:12:32, Sebastian Noack wrote: > Nit: Mind making the help messages consistent? Either add a definite article > here or remove it below. Done. https://codereview.adblockplus.org/29334114/diff/29334186/cms/bin/test_server... cms/bin/test_server.py:125: parser.add_argument('-a', '--address', default='localhost', help='Address of the interface the server will listen on') On 2016/01/21 15:12:32, Sebastian Noack wrote: > Nit: Mind making the help messages consistent? Either add a definite article > here or remove it below. Done.
LGTM
On 2016/01/21 16:14:14, Sebastian Noack wrote: > LGTM It was pointed out to me that I didn't setup my mercurial username / email correctly. Is it easy / worth it for you to re-apply the patch with this corrected?
On 2016/01/25 13:50:29, juliandoucette wrote: > On 2016/01/21 16:14:14, Sebastian Noack wrote: > > LGTM > > It was pointed out to me that I didn't setup my mercurial username / email > correctly. Is it easy / worth it for you to re-apply the patch with this > corrected? Nope, it's not important enough too mess with the repo. 
|
