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

Issue 29334114: issue 3546 - Add port and hostname options to CMS testing server (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 4 months ago by juliandoucette
Modified:
4 years, 4 months ago
Reviewers:
Sebastian Noack
CC:
kzar, saroyanm
Visibility:
Public.

Description

issue 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+16 lines, -8 lines) Patch
M cms/bin/test_server.py View 1 2 3 4 5 6 chunks +16 lines, -8 lines 0 comments Download

Messages

Total messages: 15
juliandoucette
4 years, 4 months ago (2016-01-20 13:43:47 UTC) #1
Sebastian Noack
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.py#newcode122 cms/bin/test_server.py:122: parser.add_argument('-b', '--basepath', type=str, help='Base directory (EG: /path/to/web.eyeo.com)') Please keep ...
4 years, 4 months ago (2016-01-20 14:35:59 UTC) #2
juliandoucette
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.py#newcode122 ...
4 years, 4 months ago (2016-01-20 17:19:23 UTC) #3
Sebastian Noack
Also mind the get_page function at the top of the file? The port number there ...
4 years, 4 months ago (2016-01-20 17:46:25 UTC) #4
Sebastian Noack
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.py#newcode123 cms/bin/test_server.py:123: parser.add_argument('-n', '--hostname', default='localhost', type=str, help='Server hostname (EG: localhost, example.com, ...
4 years, 4 months ago (2016-01-20 18:00:55 UTC) #5
juliandoucette
> Also mind uploading full patches when addressing comments, and not only the > changes ...
4 years, 4 months ago (2016-01-21 09:13:54 UTC) #6
Sebastian Noack
On 2016/01/21 09:13:54, juliandoucette wrote: > > Also mind uploading full patches when addressing comments, ...
4 years, 4 months ago (2016-01-21 09:55:17 UTC) #7
juliandoucette
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.py#newcode122 cms/bin/test_server.py:122: parser.add_argument('-b', '--basepath', type=str, help='Base directory (EG: /path/to/web.eyeo.com)') ...
4 years, 4 months ago (2016-01-21 13:46:24 UTC) #8
Sebastian Noack
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.py#newcode30 cms/bin/test_server.py:30: address = 'localhost' Please don't define defaults redundantly. You ...
4 years, 4 months ago (2016-01-21 14:14:09 UTC) #9
juliandoucette
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.py#newcode30 cms/bin/test_server.py:30: address = 'localhost' On 2016/01/21 14:14:09, Sebastian ...
4 years, 4 months ago (2016-01-21 14:41:36 UTC) #10
Sebastian Noack
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.py#newcode123 cms/bin/test_server.py:123: parser = argparse.ArgumentParser(description='CMS testing server.') Nit: As below, no ...
4 years, 4 months ago (2016-01-21 15:12:32 UTC) #11
juliandoucette
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.py#newcode123 cms/bin/test_server.py:123: parser = argparse.ArgumentParser(description='CMS testing server.') On 2016/01/21 15:12:32, Sebastian ...
4 years, 4 months ago (2016-01-21 16:06:26 UTC) #12
Sebastian Noack
LGTM
4 years, 4 months ago (2016-01-21 16:14:14 UTC) #13
juliandoucette
On 2016/01/21 16:14:14, Sebastian Noack wrote: > LGTM It was pointed out to me that ...
4 years, 4 months ago (2016-01-25 13:50:29 UTC) #14
Sebastian Noack
4 years, 4 months ago (2016-01-25 14:04:09 UTC) #15
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.
Sign in to reply to this message.

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