bpo-31128: Allow pydoc to bind to arbitrary hostnames #3011

feanil · 2017-08-06T22:59:20Z

https://bugs.python.org/issue31128

the-knights-who-say-ni · 2017-08-06T22:59:22Z

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!

rajathagasthya

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>

merwok · 2017-08-11T20:07:40Z

Lib/pydoc.py

-        def __init__(self, port, callback):
-            self.host = 'localhost'
+        def __init__(self, host, port, callback):
+            self.host = host


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

@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.

Ah! Thanks for noting these classes are inside a private method, that excludes compat considerations. Patch looks fine.

Mariatta · 2017-08-14T21:01:17Z

@merwok Aren't you a Core Developer? So this should not be "awaiting core review" anymore. 🤔

Mariatta · 2017-08-14T21:03:31Z

Oh I see, I think it's because you did not "approve" the PR yet ...

merwok · 2017-08-15T00:35:13Z

I used to be, but I’m not part of the github team :)

merwok · 2017-08-15T00:36:43Z

Lib/test/test_pydoc.py

@@ -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)


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.

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.

@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.

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.

Mariatta · 2017-08-15T02:14:17Z

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

brettcannon · 2017-08-15T16:27:20Z

I just need @merwok to tell me he wants to be a core dev again, then I can add him on GitHub. 😄

feanil · 2017-08-26T21:49:21Z

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

brettcannon · 2017-08-28T19:51:01Z

@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.

brettcannon

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. 😄

brettcannon · 2017-08-28T19:56:57Z

Lib/pydoc.py

@@ -2675,6 +2678,9 @@ class BadUsage(Exception): pass
 {cmd} -k <keyword>
    Search for a keyword in the synopsis lines of all available modules.

+{cmd} -h <hostname>


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.

brettcannon · 2017-08-28T19:57:18Z

Lib/test/test_pydoc.py

@@ -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)


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.

bedevere-bot · 2017-08-28T19:58:45Z

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.

merwok · 2017-08-30T17:52:04Z

@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.

brettcannon · 2017-08-30T22:00:27Z

@merwok Invite sent! (and sorry to @feanil for the noise on his PR 😄 )

This should prevent users from conflating help with the hostname.

feanil · 2017-08-31T00:05:13Z

I didn't expect the Spanish Inquisition!

bedevere-bot · 2017-08-31T00:05:15Z

Nobody expects the Spanish Inquisition!

@brettcannon, @merwok: please review the changes made to this pull request.

merwok · 2017-08-31T17:33:35Z

Misc/ACKS

@@ -1770,3 +1770,4 @@ Jelle Zijlstra
 Gennadiy Zlobin
 Doug Zongker
 Peter Åstrand
+Feanil Patel


This file is (mostly) sorted, can you move your name up with the other P?

bedevere-bot · 2017-08-31T17:33:45Z

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.

merwok · 2017-08-31T17:35:35Z

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.

feanil · 2017-09-01T01:00:49Z

I didn't expect the Spanish Inquisition!

bedevere-bot · 2017-09-01T01:00:51Z

Nobody expects the Spanish Inquisition!

@brettcannon, @merwok: please review the changes made to this pull request.

the-knights-who-say-ni added the CLA not signed label Aug 6, 2017

feanil changed the title ~~Allow pydoc to bind to arbitrary hostnames.~~ 31128-Allow pydoc to bind to arbitrary hostnames. Aug 6, 2017

feanil changed the title ~~31128-Allow pydoc to bind to arbitrary hostnames.~~ bpo-31128-Allow pydoc to bind to arbitrary hostnames. Aug 6, 2017

feanil force-pushed the fix-issue-31128 branch from 85a1c33 to dde80a4 Compare August 7, 2017 00:09

rajathagasthya approved these changes Aug 7, 2017

View reviewed changes

merwok reviewed Aug 11, 2017

View reviewed changes

bedevere-bot added the awaiting core review label Aug 11, 2017

Allow pydoc to bind to arbitrary hostnames.

d36564b

feanil force-pushed the fix-issue-31128 branch from dde80a4 to d36564b Compare August 12, 2017 02:06

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Aug 12, 2017

merwok reviewed Aug 15, 2017

View reviewed changes

merwok approved these changes Aug 15, 2017

View reviewed changes

brettcannon requested changes Aug 28, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Aug 28, 2017

brettcannon changed the title ~~bpo-31128-Allow pydoc to bind to arbitrary hostnames.~~ bpo-31128: Allow pydoc to bind to arbitrary hostnames Aug 28, 2017

merwok self-assigned this Aug 30, 2017

Switch from -h to -n for host name flag.

dc9329d

This should prevent users from conflating help with the hostname.

bedevere-bot added awaiting change review and removed awaiting changes labels Aug 31, 2017

Add news and acks.

c5b132b

merwok requested changes Aug 31, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Aug 31, 2017

merwok and others added 5 commits August 31, 2017 13:37

Minor style fix

b243c4b

Update documentation

ae519a1

Keep ACKS mostly sorted.

8a7eb0a

Update docs for pydoc.

2bbedd9

Test with a different hostname than the default.

c084751

feanil force-pushed the fix-issue-31128 branch from 8578997 to c084751 Compare September 1, 2017 00:38

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 1, 2017

merwok approved these changes Sep 1, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Sep 1, 2017

Minor change to get fresh CI status

6c6cb21

merwok merged commit 6a396c9 into python:master Sep 14, 2017

Mariatta removed the awaiting merge label Oct 8, 2017

feanil deleted the fix-issue-31128 branch March 15, 2024 18:10

Uh oh!

bpo-31128: Allow pydoc to bind to arbitrary hostnames #3011

bpo-31128: Allow pydoc to bind to arbitrary hostnames #3011

Uh oh!

Conversation

feanil commented Aug 6, 2017 • edited by brettcannon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Aug 6, 2017

Uh oh!

rajathagasthya left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mariatta commented Aug 14, 2017

Uh oh!

Mariatta commented Aug 14, 2017

Uh oh!

merwok commented Aug 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mariatta commented Aug 15, 2017

Uh oh!

brettcannon commented Aug 15, 2017

Uh oh!

feanil commented Aug 26, 2017

Uh oh!

brettcannon commented Aug 28, 2017

Uh oh!

brettcannon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 28, 2017

Uh oh!

merwok commented Aug 30, 2017

Uh oh!

brettcannon commented Aug 30, 2017

Uh oh!

feanil commented Aug 31, 2017

Uh oh!

bedevere-bot commented Aug 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 31, 2017

Uh oh!

merwok commented Aug 31, 2017

Uh oh!

feanil commented Sep 1, 2017

Uh oh!

bedevere-bot commented Sep 1, 2017

Uh oh!

Uh oh!

feanil commented Aug 6, 2017 •

edited by brettcannon

Loading

rajathagasthya left a comment •

edited

Loading