Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-34605, pty: Avoid master/slave terms #9100

Closed
wants to merge 3 commits into from
Closed

bpo-34605, pty: Avoid master/slave terms #9100

wants to merge 3 commits into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Sep 7, 2018

  • pty.spawn(): rename master_read parameter to parent_read

  • Rename pty.slave_open() to pty.child_open(), but keep an
    pty.slave_open alis to pty.child_open for backward compatibility

  • os.openpty(), os.forkpty(): rename master_fd/slave_fd
    to parent_fd/child_fd

  • Rename internal variables:

    • Rename master_fd/slave_fd to parent_fd/child_fd
    • Rename slave_name to child_name

https://bugs.python.org/issue34605

* pty.spawn(): rename master_read parameter to parent_read
* Rename pty.slave_open() to pty.child_open(), but keep an
  pty.slave_open alis to pty.child_open for backward compatibility
* os.openpty(), os.forkpty(): rename master_fd/slave_fd
  to parent_fd/child_fd
* Rename internal variables:

  * Rename master_fd/slave_fd to parent_fd/child_fd
  * Rename slave_name to child_name
@vstinner
Copy link
Member Author

vstinner commented Sep 7, 2018

pty.spawn(): rename master_read parameter to parent_read

This change is backward incompatible since technically it was possible to call pty.spawn(..., master_read=..., ...). Do we need to accept master_read as a keyword parameter but emit a deprecation warning?

Rename pty.slave_open() to pty.child_open(), but keep an pty.slave_open alis to pty.child_open for backward compatibility

I dislike keep the alias. The function isn't documented. Can't we just make the function private instead?

@vstinner
Copy link
Member Author

vstinner commented Sep 7, 2018

I didn't add a NEWS entry since I'm not sure if this PR is backward incompatible or not.

@ammaraskar
Copy link
Member

(Adding my comment from bpo here since it pertains specifically to this PR)

If you look at all the current Linux man pages and documentation, they follow the master/slave terminology. Generally, Python documentation for underlying os functions like fork, stat etc are kept short because the OS documentation is the ultimate resource for them.

This change causes a deviation from the existing standard and will only serve to make things more confusing. Anyone who goes to google about a "pty leader" will find nothing. I would suggest deferring it until the actual OS documentation reflects this change as well.

@vstinner
Copy link
Member Author

vstinner commented Sep 7, 2018

IMHO it's ok for Python to use a different terminology since "master" and "slave" are not really part of the PTY functions.

@ammaraskar
Copy link
Member

Uhh, yeah they are:

http://man7.org/linux/man-pages/man3/openpty.3.html
http://man7.org/linux/man-pages/man7/pty.7.html

@1st1 1st1 removed their request for review September 7, 2018 15:44
@vstinner
Copy link
Member Author

vstinner commented Sep 7, 2018

In C, the parameter names don't matter. You can use any name when you call the function.

In Python, "master" and "slave" are not part of the API neither. openpty() returns a tuple, that's it. The caller has to pick two names for the pair of file descriptors.

@ammaraskar
Copy link
Member

My point is, googling for pty child descriptor is going to be a far worse search than pty slave descriptor. And sure, you can pass in whatever you want for the function argument names but the default argument names are used as implicit documentation. For example, fd is conventional for file descriptors and you can search for linux fd and get lots of relevant info.

@suic86

This comment has been minimized.

@ammaraskar
Copy link
Member

Please keep motivational discussion on the bug tracker. Github is used for the technical code review.

@rhettinger
Copy link
Contributor

This change is unnecessary and ill-advised. IMO it makes the docs less usable. When "master/slave" is the actual terminology used in the problem domain, we need to keep the current wording. https://www.systutorials.com/docs/linux/man/4-pts/

@gvanrossum
Copy link
Member

Since this reflecting the terminology of the underlying UNIX pty mechanism it should not be changed.

@gvanrossum gvanrossum closed this Sep 11, 2018
@vstinner vstinner deleted the pty_master_slave branch September 13, 2018 06:34
@python python locked and limited conversation to collaborators Sep 13, 2018
@python python deleted a comment from schlamar Sep 13, 2018
@python python deleted a comment from decrepitude Sep 13, 2018
@python python deleted a comment from decrepitude Sep 13, 2018
@python python deleted a comment from owned139 Sep 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants