Skip to content

bpo-33927: Pass stdin and stdout are default arguments to argpars infile/outfile #11992

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

Merged
merged 1 commit into from
May 14, 2019

Conversation

4383
Copy link
Contributor

@4383 4383 commented Feb 22, 2019

Argparse can handle default value as stdin and stdout for parameters
as a file type (infile, outfile).

  • No issue just a minor improvement on args handeling.
  • Not a backport

https://bugs.python.org/issue33927

Argparse can handle default value as stdin and stdout for parameters
as file type (infile, outfile).
@4383 4383 changed the title Pass stdin and stdout are default arguments to argpars infile/outfile [Skip issue] Pass stdin and stdout are default arguments to argpars infile/outfile Feb 22, 2019
@vstinner
Copy link
Member

@tirkarthi: Do you want to review this change?

@tirkarthi
Copy link
Member

@vstinner Change looks good to me.

While reviewing this PR seems like a similar change is also done at least for stdout in #7865 at https://github.com/python/cpython/pull/7865/files#diff-e94945dd18482591faf1e294b029a6afR25 . It assigns '-' by default and replaces '-' with sys.stdout at https://github.com/python/cpython/pull/7865/files#diff-e94945dd18482591faf1e294b029a6afR46 or file given in the command line.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

@csabella: Do you want to merge this PR? IMHO the commit title and description should be edited. At least to remove "[Skip issue] " :-)

@vstinner
Copy link
Member

This PR looks simpler than PR #7865.

@vstinner
Copy link
Member

@csabella: Hum, maybe wait until a consensus is found with PR #7865. This PR is correct, but I'm not sure if the other one is needed or not (it's a new feature). This PR is more a straightforward cleanup.

@vstinner vstinner changed the title [Skip issue] Pass stdin and stdout are default arguments to argpars infile/outfile bpo-33927: Pass stdin and stdout are default arguments to argpars infile/outfile Feb 22, 2019
@tirkarthi
Copy link
Member

Sorry, I forgot to mention the #7865 was a feature. I have subscribed to the other and since I noticed it I wanted to convey this change might require changes and rebase in #7865 since it currently uses '-' as default to detect sys.stdout

@remilapeyre
Copy link
Contributor

Hi, I think #7865 is a bugfix and not a new feature and this PR does not fix the link bpo issue as far as I can tell

@remilapeyre
Copy link
Contributor

Hi @4383, can you clarify how is this related to the linked b.p.o report?

@4383
Copy link
Contributor Author

4383 commented Feb 22, 2019

@remilapeyre @vstinner mine isn't a bpo or a fix or something like that.
It's just a minor update to avoid to check if stdin|stdout are needed, that's all.

But mine can help to a little bit to reduce the code complexity on #7865 by avoiding to check outfile=="-" and passing default value as a sys.stdout.

@remilapeyre
Copy link
Contributor

Ok, thanks for the clarification!

There is an issue to use two FileType arguments though, they open the file when parsing command line arguments (https://github.com/python/cpython/blob/master/Lib/argparse.py#L1198).

This means that when infile and outfile refer to the same file, it will get open for writing before you read from it and its content will be lost before the program even begin:

➜  ~ echo foo >test.txt
➜  ~ python3
Python 3.7.2 (default, Jan 13 2019, 12:50:01)
[Clang 10.0.0 (clang-1000.11.45.5)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> open('test.txt')
<_io.TextIOWrapper name='test.txt' mode='r' encoding='UTF-8'>
>>> open('test.txt', 'w')
<_io.TextIOWrapper name='test.txt' mode='w' encoding='UTF-8'>
>>>
➜  ~ cat test.txt
➜  ~

I think it's not possible to use two argparse.FileType to solve the bug #33927. If we don't use FileType as the type of the infile argument, making it default to sys.stdout would not make much sense.

@4383
Copy link
Contributor Author

4383 commented Feb 22, 2019

@remilapeyre yeah good point argparse.FileType on outfile is an issue. But default value can keep sys.stdout and you just have to check if type is <class '_io.TextIOWrapper'> instead check if value is equal to -.

I guess is a little bit more proper to assume that the default value is the sys.stdout instead check for string who are not really formalized anyway.

Thoughts?

@4383
Copy link
Contributor Author

4383 commented Feb 22, 2019

With an <class '_io.TextIOWrapper'> object as the default value you are really sure that user want to display output directly on stdout`

@4383
Copy link
Contributor Author

4383 commented Feb 22, 2019

@remilapeyre And you can continue your fix by removing argparse.FileType as a type on outfile and maybe use type as str.

Cf. my comments on yours #7865

@4383
Copy link
Contributor Author

4383 commented May 14, 2019

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

@vstinner vstinner merged commit 4d45a3b into python:master May 14, 2019
@vstinner
Copy link
Member

Thanks, I merged your PR.

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.

6 participants