classification
Title: argparse FileType raises ugly exception for missing file
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: bethard Nosy List: SilentGhost, Tarsis.Azevedo, akira, bethard, doughellmann, eric.araujo, georg.brandl
Priority: normal Keywords: easy, patch

Created on 2010-08-04 13:08 by doughellmann, last changed 2011-01-25 13:43 by bethard. This issue is now closed.

Files
File name Uploaded Description Edit
argparse_filetype_error.py doughellmann, 2010-08-04 13:08
argparse_int_error.py doughellmann, 2010-08-04 13:08
issue9509.diff Tarsis.Azevedo, 2010-11-21 15:38
issue9509.patch akira, 2010-11-21 23:29 patch for py3k r86649
test_argparse.diff akira, 2010-11-25 03:30 test readonly files, patch for py3k r86743
argparse.diff akira, 2010-11-25 03:40 patch for py3k r86743
argparse.diff SilentGhost, 2011-01-19 19:40
argparse.diff bethard, 2011-01-23 11:25
Messages (31)
msg112798 - (view) Author: Doug Hellmann (doughellmann) * (Python committer) Date: 2010-08-04 13:08
Most of the argparse type converters handle exceptions with a single line error message explaining the problem.  For example, if an argument -i is declared as an int, but the value given ('a') cannot be converted to an int, the message is something like "argument -i: invalid int value: 'a'".  

On the other hand, FileType raises an IOError, which isn't being caught and converted to the simpler error message. Instead, a full traceback is printed.  To be consistent, the IOError should be trapped and a single error message printed.
msg121941 - (view) Author: Tarsis Azevedo (Tarsis.Azevedo) Date: 2010-11-21 15:38
Hi Doug,

I catched the IOError exception and used the error function of ArgumentParser class to return a beautful error message.
msg121946 - (view) Author: Doug Hellmann (doughellmann) * (Python committer) Date: 2010-11-21 15:56
Hi, Tarsis,

That patch looks good to me.

Thanks!
Doug
msg121987 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-11-21 20:53
Looks good to me too.  Steven, does this require a test?
msg121995 - (view) Author: Doug Hellmann (doughellmann) * (Python committer) Date: 2010-11-21 21:13
Oh, yeah, a test is a good idea.
msg122019 - (view) Author: Akira Li (akira) * Date: 2010-11-21 23:29
Simplified the patch and added test for a non-existent file in issue9509.patch

The test `py3k/python -m test.regrtest test_argparse` fails without the patch and succeeds with it.

The docs in Doc/library/argparse.rst and Lib/argparse.py  don't need to be changed (neither mention IOError and it is a buf fix).

Misc/NEWS and Misc/ACKS don't need to be changed (?).

I've uploaded the patch to Rietveld http://codereview.appspot.com/3251041/
msg122026 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-11-21 23:57
I don’t know enough of the test_argparse magic to assess the new test, but Stephan, the author and maintainer of argparse, will certainly comment when he gets some time.  Thanks for the patch!
msg122300 - (view) Author: Steven Bethard (bethard) * (Python committer) Date: 2010-11-24 20:13
I'm not sure about the patch - this will convert *all* IOErrors into command line error messages, while we should really only be converting the ones raised by FileType. Instead, the try/except should be in FileType.__call__, and you should raise an ArgumentTypeError there.

The test looks fine.
msg122322 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2010-11-25 00:34
Attached patch with the try-except clause as suggested by Steven, Doug's example now produces the following output:

$ ./python argparse_filetype_error.py
usage: argparse_filetype_error.py [-h] [-i I]
argparse_filetype_error.py: error: no such file or directory 'file-does-not-exist.txt'

I have digressed and fixed an issue with _bufsize 0. I thought it would be just natural to default to -1 which is default buffering size for a simple open call anyway. It also makes for a cleaner try-except clause. All tests pass, including akira's.
msg122325 - (view) Author: Akira Li (akira) * Date: 2010-11-25 03:30
I've added tests for readonly files. SilentGhost's patch doesn't handle this case.
msg122326 - (view) Author: Akira Li (akira) * Date: 2010-11-25 03:40
Attached patch for argparse.py (argparse.diff -- without tests, see tests in test_argparse.diff) where ValueError is raises in FileType.__call__ and additional details printed on ArgumentError
msg122327 - (view) Author: Akira Li (akira) * Date: 2010-11-25 04:08
updated patch on http://codereview.appspot.com/3251041/
msg122354 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2010-11-25 11:56
Ammended akira's patch for Lib/test/test_argparse.py to include suggested in review changes: with statement, import statement
msg122468 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2010-11-26 17:25
On windows proposed changes to Lib/test/test_argparse.py cause it to enter an infinite loop in TempDirMixin.tearDown method.

As it seemed exclusively Windows issue, this new patch replaces while loop with the ignore_errors parameter for shutil.rmtree.

Now all test pass.
msg122527 - (view) Author: Steven Bethard (bethard) * (Python committer) Date: 2010-11-27 14:46
Tried to comment in Rietveld but it didn't work for some reason. Anyway, I think the argparse.py patch isn't good - changing the type error message to "'invalid %s value: %r details: "%s"'" will change the behavior of a bunch of programs, and it's not clearly for the better. Instead, you should raise an ArgumentTypeError instead of a ValueError, and give it whatever message you want there. That is, let's keep this patch local to the FileType, and not touch the rest of argparse.
msg122528 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2010-11-27 14:52
Steven, I'm not sure why you're insisting on ArgumentTypeError, when it should be ArgumentError. The file name is not coerced into a different file type, but rather the error occurs when trying to use parameter passed.

In any way, my patch is still available. Do you not like something about it?
msg122560 - (view) Author: Steven Bethard (bethard) * (Python committer) Date: 2010-11-27 21:34
Sorry, I was looking at the akira patch with the same date, where I was mainly worried about the modification of the "except (TypeError, ValueError):" block. Your patch doesn't do that, and looks fine.
msg126543 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-01-19 17:17
SilentGhost, can you remove old files from the report (or tell which ones I should remove if you can’t do it) and make one patch with code and test changes that apply cleanly to current py3k?  That would make a final review by Steven easier.
msg126553 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2011-01-19 18:28
Here is the single patch. All tests pass.
msg126558 - (view) Author: Akira Li (akira) * Date: 2011-01-19 19:32
"no such file or directory '%s'" is misleading if you are trying to open a readonly file for writing.

The message might be replace by: "can't open '%s'"
msg126880 - (view) Author: Steven Bethard (bethard) * (Python committer) Date: 2011-01-23 10:11
Georg, is this something we can patch for rc2? It's a bug - errors encountered by argparse-internal code should be translated into command line errors, and they currently aren't for read-only files.

For what it's worth, the tests fail when the new test_argparse test is added, and pass when the patch is applied, and I have personally reviewed the code and run the entire test suite on OS X and everything still passes.
msg126881 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2011-01-23 10:14
This would be acceptable to patch; I wonder whether to swallow the exception text of IOError though.
msg126882 - (view) Author: Steven Bethard (bethard) * (Python committer) Date: 2011-01-23 11:25
Good point. Here's the updated patch that reports the IOError as well. All tests pass. I'll apply in a bit if I don't hear otherwise.
msg126884 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2011-01-23 11:36
Library patch looks good.

Tests: Does the create_readonly_file helper work on all platforms, esp. Windows?  Maybe it's better to create a different error situation?
msg126886 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2011-01-23 11:40
I've tested this on windows. It passed all test.
msg126889 - (view) Author: Steven Bethard (bethard) * (Python committer) Date: 2011-01-23 11:55
The docs for os.chmod claim:

Availability: Unix, Windows.
Although Windows supports chmod(), you can only set the file's read-only flag with it (via the stat.S_IWRITE and stat.S_IREAD constants or a corresponding integer value). All other bits are ignored.

So at least according to the current docs, we should be okay calling `os.chmod(file_path, stat.S_IREAD)` on Unix and Windows (and of course OS X too). Is that enough or did you have other platforms in mind?
msg126945 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2011-01-24 19:36
No, it sounds fine then.
msg126956 - (view) Author: Steven Bethard (bethard) * (Python committer) Date: 2011-01-24 21:06
Fixed in r88169 and r88171. Thanks everyone for your help! I'll be keeping my eye on the buildbots for a bit to make sure everything stays green.
msg126959 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-01-24 21:49
Oops, I missed that while reading the patch: It introduces a translatable string which may cause i18n issues (discussed in #10528).  I can propose a patch this week if no-one beats me to it.
msg126996 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2011-01-25 11:10
Steven, I'm wondering why do you raise ArgumentTypeError there? From reading doc strings of the relevant errors, it seems obvious to me that it should be ArgumentError. Argument is being used there, there's no conversion occurring anywhere.
msg127007 - (view) Author: Steven Bethard (bethard) * (Python committer) Date: 2011-01-25 13:43
It's an ArgumentTypeError because that's what you're supposed to raise inside type functions:

http://docs.python.org/dev/library/argparse.html#type

(Note that argparse.FileType.__call__ is what will be called when we pass type=argparse.FileType(...) to add_argument.)

The current docstring for ArgumentTypeError is correct - it says it's "An error from trying to convert a command line string to a type" and we're converting a file path (string) into a file object.  But I certainly wouldn't complain if someone wanted to make the wording there clearer. Basically the rule is:

* Use ArgumentTypeError when you're defining a type= function
* Use ArgumentError otherwise
History
Date User Action Args
2011-01-25 13:43:20bethardsetnosy: georg.brandl, bethard, eric.araujo, doughellmann, SilentGhost, akira, Tarsis.Azevedo
messages: + msg127007
2011-01-25 11:10:35SilentGhostsetnosy: georg.brandl, bethard, eric.araujo, doughellmann, SilentGhost, akira, Tarsis.Azevedo
messages: + msg126996
2011-01-24 21:49:52eric.araujosetnosy: georg.brandl, bethard, eric.araujo, doughellmann, SilentGhost, akira, Tarsis.Azevedo
messages: + msg126959
2011-01-24 21:06:09bethardsetstatus: languishing -> closed
nosy: georg.brandl, bethard, eric.araujo, doughellmann, SilentGhost, akira, Tarsis.Azevedo
messages: + msg126956

assignee: bethard
resolution: fixed
stage: patch review -> resolved
2011-01-24 19:36:46georg.brandlsetnosy: georg.brandl, bethard, eric.araujo, doughellmann, SilentGhost, akira, Tarsis.Azevedo
messages: + msg126945
2011-01-23 11:55:58bethardsetnosy: georg.brandl, bethard, eric.araujo, doughellmann, SilentGhost, akira, Tarsis.Azevedo
messages: + msg126889
2011-01-23 11:40:40SilentGhostsetnosy: georg.brandl, bethard, eric.araujo, doughellmann, SilentGhost, akira, Tarsis.Azevedo
messages: + msg126886
2011-01-23 11:36:59georg.brandlsetnosy: georg.brandl, bethard, eric.araujo, doughellmann, SilentGhost, akira, Tarsis.Azevedo
messages: + msg126884
2011-01-23 11:25:59bethardsetfiles: + argparse.diff
nosy: georg.brandl, bethard, eric.araujo, doughellmann, SilentGhost, akira, Tarsis.Azevedo
messages: + msg126882
2011-01-23 10:14:55georg.brandlsetnosy: georg.brandl, bethard, eric.araujo, doughellmann, SilentGhost, akira, Tarsis.Azevedo
messages: + msg126881
2011-01-23 10:11:36bethardsetnosy: + georg.brandl
messages: + msg126880
2011-01-19 19:40:53SilentGhostsetfiles: - argparse.diff
nosy: bethard, eric.araujo, doughellmann, SilentGhost, akira, Tarsis.Azevedo
2011-01-19 19:40:49SilentGhostsetfiles: + argparse.diff
nosy: bethard, eric.araujo, doughellmann, SilentGhost, akira, Tarsis.Azevedo
2011-01-19 19:32:04akirasetnosy: bethard, eric.araujo, doughellmann, SilentGhost, akira, Tarsis.Azevedo
messages: + msg126558
2011-01-19 18:28:49SilentGhostsetfiles: - argparse.py.diff
nosy: bethard, eric.araujo, doughellmann, SilentGhost, akira, Tarsis.Azevedo
2011-01-19 18:28:41SilentGhostsetfiles: - test_argparse.py.diff
nosy: bethard, eric.araujo, doughellmann, SilentGhost, akira, Tarsis.Azevedo
2011-01-19 18:28:33SilentGhostsetfiles: + argparse.diff
nosy: bethard, eric.araujo, doughellmann, SilentGhost, akira, Tarsis.Azevedo
messages: + msg126553
2011-01-19 17:17:55eric.araujosetnosy: bethard, eric.araujo, doughellmann, SilentGhost, akira, Tarsis.Azevedo
messages: + msg126543
2011-01-19 15:44:36SilentGhostsetstatus: open -> languishing
nosy: bethard, eric.araujo, doughellmann, SilentGhost, akira, Tarsis.Azevedo
2010-11-27 21:34:41bethardsetmessages: + msg122560
2010-11-27 14:52:30SilentGhostsetmessages: + msg122528
2010-11-27 14:46:40bethardsetmessages: + msg122527
2010-11-26 17:25:51SilentGhostsetfiles: + test_argparse.py.diff

messages: + msg122468
2010-11-26 17:23:37SilentGhostsetfiles: - test_argparse.py.diff
2010-11-25 11:56:06SilentGhostsetfiles: + test_argparse.py.diff

messages: + msg122354
2010-11-25 04:08:55akirasetmessages: + msg122327
2010-11-25 03:40:56akirasetfiles: + argparse.diff

messages: + msg122326
2010-11-25 03:30:59akirasetfiles: + test_argparse.diff

messages: + msg122325
2010-11-25 00:34:57SilentGhostsetfiles: + argparse.py.diff
nosy: + SilentGhost
messages: + msg122322

2010-11-24 20:13:44bethardsetmessages: + msg122300
2010-11-21 23:57:16eric.araujosetmessages: + msg122026
2010-11-21 23:29:21akirasetfiles: + issue9509.patch

nosy: + akira
messages: + msg122019

type: behavior
2010-11-21 21:13:25doughellmannsetmessages: + msg121995
2010-11-21 20:53:10eric.araujosetnosy: + eric.araujo

messages: + msg121987
stage: patch review
2010-11-21 15:56:08doughellmannsetmessages: + msg121946
2010-11-21 15:38:13Tarsis.Azevedosetfiles: + issue9509.diff

nosy: + Tarsis.Azevedo
messages: + msg121941

keywords: + patch
2010-09-07 20:57:40r.david.murraysetnosy: + bethard
2010-08-05 11:26:21doughellmannsetkeywords: + easy
2010-08-04 13:08:17doughellmannsetfiles: + argparse_int_error.py
2010-08-04 13:08:06doughellmanncreate