bpo-31128: Allow pydoc to bind to arbitrary hostnames by feanil · Pull Request #3011 · python/cpython (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation28 Commits9 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Hello, and thanks for your contribution!
I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).
Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.
Thanks again to your contribution and we look forward to looking at it!
Allow pydoc to bind to arbitrary hostnames. 31128-Allow pydoc to bind to arbitrary hostnames.
31128-Allow pydoc to bind to arbitrary hostnames. bpo-31128-Allow pydoc to bind to arbitrary hostnames.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Verified locally:
~/dev/cpython $ ./python.exe -m pydoc -h localhost -p 5000
Server ready at http://localhost:5000/
Server commands: [b]rowser, [q]uit
server>
| def __init__(self, port, callback): |
|---|
| self.host = 'localhost' |
| def __init__(self, host, port, callback): |
| self.host = host |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this class is considered part of the module’s public API, this change would break compatibility. Alternative ways to achieve the same result could be:
- add host after the existing params, with default value to keep compat
- allow the existing port to be a (host, port) tuple
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@merwok, I considered that but figured it was a class inside a underscored method so its not guaranteed to remain compatible. I figured it would be better to opt for the more readable signature instead of being backwards compatible. If it's a blocker to make this compatible let me know but if we're implying that this function should be treated as if it is public we may want to remove the prefixed underscore but that will definitely break people.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Thanks for noting these classes are inside a private method, that excludes compat considerations. Patch looks fine.
@merwok Aren't you a Core Developer? So this should not be "awaiting core review" anymore. 🤔
Oh I see, I think it's because you did not "approve" the PR yet ...
I used to be, but I’m not part of the github team :)
| @@ -913,7 +913,7 @@ def my_url_handler(url, content_type): |
|---|
| text = 'the URL sent was: (%s, %s)' % (url, content_type) |
| return text |
| serverthread = pydoc._start_server(my_url_handler, port=0) |
| serverthread = pydoc._start_server(my_url_handler, hostname='localhost', port=0) |
| self.assertIn('localhost', serverthread.docserver.address) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically this test does not validate that the patch is correct, since it would also pass without the code change! But it’s probably fine for this patch.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is localhost specified anywhere else in the pydoc.py file? If it isn't then while you're right it isn't testing a difference from the default from the public API, this does help make sure that at least the value is being used.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brettcannon just making sure you don't want me to do anything here? The one place that 'localhost' is used in pydoc.py is for the default vaule of the hostname parameter for the browse() function to not break backwards compatibility.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment before Brett’s: technically, this test does not prove that the patch is correct, since 'localhost' is the value that was used before your changes.
@merwok I see that you are still a committer in b.p.o. I hope you'll continue contributing as core developer 😄 If interested, perhaps check with @brettcannon of what needs to happen?
I just need @merwok to tell me he wants to be a core dev again, then I can add him on GitHub. 😄
Checking back in on this, I just wanted to confirm that there is nothing left to do on my end for this, and that it's just waiting for someone on the core team to review and merge(hopefully)?
@feanil yep, the awaiting core review label means you're waiting on a core dev to review this and either ask for changes or merge it.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good, just a question of whether the CLI switch is the right letter.
Also don't forget to add yourself to Misc/ACKS and to write a news entry. 😄
| @@ -2675,6 +2678,9 @@ class BadUsage(Exception): pass |
|---|
| {cmd} -k |
| Search for a keyword in the synopsis lines of all available modules. |
| {cmd} -h |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using -h is a bit dodgy since it's so commonly used as shorthand for --help. Unfortunately I don't have a better suggestion beyond -n.
| @@ -913,7 +913,7 @@ def my_url_handler(url, content_type): |
|---|
| text = 'the URL sent was: (%s, %s)' % (url, content_type) |
| return text |
| serverthread = pydoc._start_server(my_url_handler, port=0) |
| serverthread = pydoc._start_server(my_url_handler, hostname='localhost', port=0) |
| self.assertIn('localhost', serverthread.docserver.address) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is localhost specified anywhere else in the pydoc.py file? If it isn't then while you're right it isn't testing a difference from the default from the public API, this does help make sure that at least the value is being used.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.
Once you have made the requested changes, please leave a comment on this pull request containing the phrase I didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
bpo-31128-Allow pydoc to bind to arbitrary hostnames. bpo-31128: Allow pydoc to bind to arbitrary hostnames
@brettcannon Alright please add me to the team! I can at least review distutils PRs since not many other people would do that, and hopefully get back to contributing.
@merwok Invite sent! (and sorry to @feanil for the noise on his PR 😄 )
This should prevent users from conflating help with the hostname.
I didn't expect the Spanish Inquisition!
| @@ -1770,3 +1770,4 @@ Jelle Zijlstra |
|---|
| Gennadiy Zlobin |
| Doug Zongker |
| Peter Åstrand |
| Feanil Patel |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is (mostly) sorted, can you move your name up with the other P?
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.
Once you have made the requested changes, please leave a comment on this pull request containing the phrase I didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
By the way, is the command-line interface documented in the reST docs? If so, please add the new option there too. You could mention that it’s useful in containers, since that was the motivating use case.
I didn't expect the Spanish Inquisition!
