The Wayback Machine - https://web.archive.org/web/20220505115902/https://github.com/python/cpython/pull/92016
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

gh-82678: Fix parameter name for pipes #92016

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

Copy link
Contributor

@slateny slateny commented Apr 28, 2022

#82678

https://github.com/python/cpython/blob/main/Lib/pipes.py#L145

(Also a random # ' here that could optionally be removed)

fp = t.open(file, mode)
where mode is 'r' to read the file, or 'w' to write it -- just like
fp = t.open(file, rw)
where rw is 'r' to read the file, or 'w' to write it -- just like
Copy link
Contributor

@eendebakpt eendebakpt Apr 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
where rw is 'r' to read the file, or 'w' to write it -- just like
where 'rw' is 'r' to read the file, or 'w' to write it -- just like

Copy link
Contributor Author

@slateny slateny Apr 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I wonder if making it a string could be misleading - without quotes, it's clear that it's a variable of some sort, while with, maybe it gets confused as a value

Copy link
Member

@AlexWaygood AlexWaygood May 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A compromise could be:

Suggested change
where rw is 'r' to read the file, or 'w' to write it -- just like
where *rw* is 'r' to read the file, or 'w' to write it -- just like

But the way @slateny has it currently is in keeping with the format used in the rest of the docstring: infile and outfile on line 46 are not surrounded by quotes or asterisks.

Copy link
Contributor Author

@slateny slateny May 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah asterisks are a good idea, and good point on the consistency. I'm fine with both so I'll defer the choice here

@eendebakpt
Copy link

@eendebakpt eendebakpt commented Apr 28, 2022

@slateny The argument rw is not documented in the method open itself. That might be a better place to document that the value should be r or w. And would it be ok to add type annotations? E.g.

def open(self, file: str, rw : str):

@slateny
Copy link
Author

@slateny slateny commented Apr 28, 2022

Not sure what the conventions are when using type annotations - I just did the smallest change possible to fix the bug. I imagine that it's not noted directly under the method docstring because the implementation seems straightforward enough

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants