bpo-36345: Include the code from Tools/scripts/serve.py for the wsgiref-base web server example#12562
Conversation
Sorry, something went wrong.
matrixise
force-pushed
the
bpo-36345-part-3
branch
from
d176d16 to
30b5249
Compare
March 26, 2019 14:13
|
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? |
Sorry, something went wrong.
|
I have made the requested changes; please review again |
Sorry, something went wrong.
|
Thanks for making the requested changes! : please review the changes made to this pull request. |
Sorry, something went wrong.
|
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
|
Sorry, something went wrong.
…ef-base web server example
optional directory and port number on the command line.
matrixise
force-pushed
the
bpo-36345-part-3
branch
from
8d3c270 to
a3dbd35
Compare
April 15, 2019 12:23
|
I have made the requested changes; please review again |
Sorry, something went wrong.
|
Thanks for making the requested changes! : please review the changes made to this pull request. |
Sorry, something went wrong.
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/?
Sorry, something went wrong.
|
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.
👍
|
Sorry, something went wrong.
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 ;-)
Sorry, something went wrong.
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.
Sorry, something went wrong.
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 :-) |
Sorry, something went wrong.
|
@berkerpeksag Thank you for your advice, which I will use next time. |
Sorry, something went wrong.
I'm not sure how that is an argument here? It could be easily noticed and fixed before merging the PR. @matrixise welcome! |
Sorry, something went wrong.
Reviewers
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