bpo-36345: Include the code from Tools/scripts/serve.py for the wsgiref-base web server example #12562

matrixise · 2019-03-26T14:10:29Z

In this PR, I include the code from Tools/scripts/serve.py file 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

matrixise · 2019-03-26T14:10:56Z

@vstinner feel free to review this PR. Thank you so much

vstinner · 2019-03-26T14:45:45Z

make check suspicious html SPHINXOPTS="-q -W -j4" failed with the following error on Travis CI:

Warning, treated as error:
[whatsnew/changelog:28] ".. literalinclude:" found in ".. literalinclude::"

I'm not sure why literalinclude is seen as an error?

@JulienPalard: Any idea?

matrixise · 2019-04-04T09:50:07Z

@JulienPalard @vstinner ?

I have made the requested changes; please review again

bedevere-bot · 2019-04-04T09:50:10Z

Thanks for making the requested changes!

: please review the changes made to this pull request.

vstinner · 2019-04-08T10:56:40Z

Doc/library/wsgiref.rst

-
-        # Serve until process is killed
-        httpd.serve_forever()
+Example of a small wsgiref-based web server


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 · 2019-04-15T11:49:17Z

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.

matrixise · 2019-04-15T12:23:59Z

I have made the requested changes; please review again

bedevere-bot · 2019-04-15T12:24:01Z

Thanks for making the requested changes!

: please review the changes made to this pull request.

vstinner

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/?

vstinner · 2019-04-15T16:04:18Z

Tools/scripts/serve.py

@@ -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 · 2019-04-15T16:05:08Z

Doc/library/wsgiref.rst

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


Suggested change

directory and port number on the command line:

directory and port number (default: 8000) on the command line:

matrixise · 2019-04-15T19:35:31Z

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

LGTM. You can merge it once you will be able to merge changes ;-)

berkerpeksag

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.rst even if you're a core developer, though.

vstinner · 2019-04-16T14:17:01Z

Only big documentation changes require a NEWS entry.

I'm wasn't sure on that point. In case of doubt, I said nothing :-)

We only attribute contributors in NEWS entries. It's OK to add your name to Doc/whatsnew/X.Y.rst even if you're a core developer, though.

Well, Stéphane wrote this PR before he was a core dev :-)

matrixise · 2019-04-16T14:28:40Z

@berkerpeksag Thank you for your advice, which I will use next time.

berkerpeksag · 2019-04-16T21:14:03Z

Well, Stéphane wrote this PR before he was a core dev :-)

I'm not sure how that is an argument here? It could be easily noticed and fixed before merging the PR.

@matrixise welcome!

bedevere-bot added the docs Documentation in the Doc dir label Mar 26, 2019

the-knights-who-say-ni added the CLA signed label Mar 26, 2019

bedevere-bot added the awaiting review label Mar 26, 2019

matrixise force-pushed the bpo-36345-part-3 branch from d176d16 to 30b5249 Compare March 26, 2019 14:13

bedevere-bot added awaiting change review and removed awaiting review labels Apr 4, 2019

vstinner reviewed Apr 8, 2019

View reviewed changes

matrixise added 4 commits April 15, 2019 13:53

bpo-36345: Include the code from Tools/scripts/serve.py for the wsgir…

756332d

…ef-base web server example

Fix the blurb entry

74fd34c

Example of a WSGI application serving the current directory, accept

c938813

optional directory and port number on the command line.

Update the description of the example

a3dbd35

matrixise force-pushed the bpo-36345-part-3 branch from 8d3c270 to a3dbd35 Compare April 15, 2019 12:23

vstinner reviewed Apr 15, 2019

View reviewed changes

matrixise added 3 commits April 15, 2019 21:38

Add the default port in the doc

530ed32

Close the resources for the httpd server

2b1fa72

Remove \b\b for the portability

a7db659

vstinner approved these changes Apr 15, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Apr 15, 2019

matrixise merged commit 2b7f93b into python:master Apr 16, 2019

bedevere-bot removed the awaiting merge label Apr 16, 2019

matrixise deleted the bpo-36345-part-3 branch April 16, 2019 12:57

berkerpeksag reviewed Apr 16, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-36345: Include the code from Tools/scripts/serve.py for the wsgiref-base web server example #12562

bpo-36345: Include the code from Tools/scripts/serve.py for the wsgiref-base web server example #12562

matrixise commented Mar 26, 2019 •

edited by bedevere-bot

Loading

matrixise commented Mar 26, 2019

vstinner commented Mar 26, 2019

matrixise commented Apr 4, 2019

bedevere-bot commented Apr 4, 2019

vstinner Apr 8, 2019

matrixise commented Apr 15, 2019 via email

matrixise commented Apr 15, 2019

bedevere-bot commented Apr 15, 2019

vstinner left a comment

vstinner Apr 15, 2019

matrixise Apr 15, 2019

vstinner Apr 15, 2019

matrixise commented Apr 15, 2019 via email

vstinner left a comment

berkerpeksag left a comment

vstinner commented Apr 16, 2019

matrixise commented Apr 16, 2019

berkerpeksag commented Apr 16, 2019

	directory and port number on the command line:
	directory and port number (default: 8000) on the command line:

bpo-36345: Include the code from Tools/scripts/serve.py for the wsgiref-base web server example #12562

bpo-36345: Include the code from Tools/scripts/serve.py for the wsgiref-base web server example #12562

Conversation

matrixise commented Mar 26, 2019 • edited by bedevere-bot Loading

matrixise commented Mar 26, 2019

vstinner commented Mar 26, 2019

matrixise commented Apr 4, 2019

bedevere-bot commented Apr 4, 2019

vstinner Apr 8, 2019

Choose a reason for hiding this comment

matrixise commented Apr 15, 2019 via email

matrixise commented Apr 15, 2019

bedevere-bot commented Apr 15, 2019

vstinner left a comment

Choose a reason for hiding this comment

vstinner Apr 15, 2019

Choose a reason for hiding this comment

matrixise Apr 15, 2019

Choose a reason for hiding this comment

vstinner Apr 15, 2019

Choose a reason for hiding this comment

matrixise commented Apr 15, 2019 via email

vstinner left a comment

Choose a reason for hiding this comment

berkerpeksag left a comment

Choose a reason for hiding this comment

vstinner commented Apr 16, 2019

matrixise commented Apr 16, 2019

berkerpeksag commented Apr 16, 2019

matrixise commented Mar 26, 2019 •

edited by bedevere-bot

Loading