bpo-36345: Include the code from Tools/scripts/serve.py for the wsgiref-base web server example#12562
Uh oh!
There was an error while loading. Please reload this page.
Conversation
matrixise commented Mar 26, 2019
@vstinner feel free to review this PR. Thank you so much |
matrixiseforce-pushed the bpo-36345-part-3 branch from d176d16 to 30b5249Compare
vstinner commented Mar 26, 2019
make check suspicious html SPHINXOPTS="-q -W -j4" failed with the following error on Travis CI: I'm not sure why literalinclude is seen as an error? @JulienPalard: Any idea? |
matrixise commented Apr 4, 2019
I have made the requested changes; please review again |
bedevere-bot commented Apr 4, 2019
Thanks for making the requested changes! : please review the changes made to this pull request. |
Doc/library/wsgiref.rst Outdated
There was a problem hiding this comment.
Can you explain what the server does? It seems like it serves the directory passed as the first argument on the command line, and accept an optional port number as the second parameter. Proposition:
"Example of a WSGI application serving the directory passed on the command line:"
By the way, would it make sense to modify the example of use the current directory by default, rather than require to pass a directory?
path = sys.argv[1] if len(sys.argv) > 1 else os.getcwd() with:
"Example of a WSGI application serving the current directory, accept optional directory and port number on the command line:"
matrixise commented Apr 15, 2019 via email
By the way, would it make sense to modify the example of use the current directory by default, rather than require to pass a directory? ``` path = sys.argv[1] if len(sys.argv) > 1 else os.getcwd() ``` with: "Example of a WSGI application serving the current directory, accept optional directory and port number on the command line:" I am fine with the optional directory, and because it's an example, it's not a problem, we don't break the backward compatibility. +1 |
…ef-base web server example
optional directory and port number on the command line.
matrixiseforce-pushed the bpo-36345-part-3 branch from 8d3c270 to a3dbd35Comparematrixise commented Apr 15, 2019
I have made the requested changes; please review again |
bedevere-bot commented Apr 15, 2019
Thanks for making the requested changes! : please review the changes made to this pull request. |
vstinner left a comment
vstinner left a comment There was a problem hiding this comment.
When I tested the example, it wasn't obvious to me how to access the server.
Maybe the script should display somehow the URL http://localhost:8000/?
| path=sys.argv[1]iflen(sys.argv) >1elseos.getcwd() | ||
| port=int(sys.argv[2]) iflen(sys.argv) >2else8000 | ||
| httpd=simple_server.make_server('', port, app) | ||
| print("Serving{} on port{}, control-C to stop".format(path, port)) |
There was a problem hiding this comment.
When I press CTRL+C, I get a ResourceWarning. Please add httpd.server_close() after print("\b\bShutting down."). By the way, I'm not sure that "\b" is supported on Windows. I suggest to remove ""\b\b" to make the script more portable.
Doc/library/wsgiref.rst Outdated
| # Serve until process is killed | ||
| httpd.serve_forever() | ||
| Example of a WSGI application serving the current directory, accept optional | ||
| directory and port number on the command line: |
There was a problem hiding this comment.
| directory and port number on the command line: | |
| directory and port number (default: 8000) on the command line: |
matrixise commented Apr 15, 2019 via email
When I tested the example, it wasn't obvious to me how to access the server. Yep, it's the default output of `Tools/scripts/serve.py` Maybe the script should display somehow the URL http://localhost:8000/? Like the output of python -m http.server -d DIRECTORY? I propose to submit an other bpo issue/pull request where we suggest to change the default output of the `Tools/scripts/serve.py` What do you think? > @@ -25,7 +25,7 @@ def app(environ, respond): return [b'not found'] if __name__ == '__main__': - path = sys.argv[1] + path = sys.argv[1] if len(sys.argv) > 1 else os.getcwd() port = int(sys.argv[2]) if len(sys.argv) > 2 else 8000 httpd = simple_server.make_server('', port, app) print("Serving{} on port{}, control-C to stop".format(path, port)) When I press CTRL+C, I get a ResourceWarning. Please add httpd.server_close() after print("\b\bShutting down."). By the way, I'm not sure that "\b" is supported on Windows. I suggest to remove "\b\b" to make the script more portable. 👍 |
vstinner left a comment
vstinner left a comment There was a problem hiding this comment.
LGTM. You can merge it once you will be able to merge changes ;-)
berkerpeksag left a comment
berkerpeksag left a comment There was a problem hiding this comment.
Two things:
- Only big documentation changes require a NEWS entry.
- We only attribute contributors in NEWS entries. It's OK to add your name to
Doc/whatsnew/X.Y.rsteven if you're a core developer, though.
vstinner commented Apr 16, 2019
I'm wasn't sure on that point. In case of doubt, I said nothing :-)
Well, Stéphane wrote this PR before he was a core dev :-) |
matrixise commented Apr 16, 2019
@berkerpeksag Thank you for your advice, which I will use next time. |
berkerpeksag commented Apr 16, 2019
I'm not sure how that is an argument here? It could be easily noticed and fixed before merging the PR. @matrixise welcome! |
5 participants
In this PR, I include the code from
Tools/scripts/serve.pyfile but I don't touch the code of the script, just keep it intact because it's not the role of this PR.https://bugs.python.org/issue36345