try:
if options.input_file:
infile = open(options.input_file, 'r')
else:
infile = sys.stdin
if options.output_file:
outfile = open(options.output_file, 'w')
else:
outfile = sys.stdout
for line in infile:
# ...
except IOError, e:
print "Can't open file %s." % e.filename
sys.exit(e.errno)
finally:
if options.input_file: infile.close()
if options.output_file: outfile.close()
Refactorings
No refactoring yet !
Dave Cridland
August 28, 2008, August 28, 2008 07:56, permalink
Well, the finally block isn't important - Python closes files when the object lifetime ends - basically doing what you want. Besides which, closing stdin or stdout isn't a disaster anyway, it's pretty normal behaviour.
The except block is reporting the wrong error - you might have got IOError during reading or writing, after all - in general, whilst I like having a catch-all exception handler, Python has it anyway, so catch specific errors where they occur.
sys.exit should indicate what the error is, not what the error code was. In you case, it'll return, for example, EPERM, without indication of whether it's the input or output file at fault.
I prefer wrapping main code in a traditional if __name__ == '__main__', at least if the code is at all likely to end up being imported.
And by the way, PEP-8 applies to code within the standard library, rather than standalone scripts. A lot of it is helpful, of course, even here.
if __name__ == '__main__':
try:
if options.input_file:
infile = open(options.input_file, 'r')
else:
infile = sys.stdin
except IOError, e:
print >>sys.stderr, "Cannot open input file %s." % (options.input_file)
sys.exit(1)
try:
if options.output_file:
outfile = open(options.output_file, 'w')
else:
outfile = sys.stdout
except IOError, e:
print >>sys.stderr, "Cannot open output file %s." % (options.output_file)
sys.exit(2)
for line in infile:
# ...
Malte
August 28, 2008, August 28, 2008 22:17, permalink
I agree with most changes here, but I don't think the exit code should differ depending on what kind of error you had. It's customary to have 2 for bad usage (i.e. errors parsing the command-line arguments) and 1 for essentially all other errors, so sys.exit actually has the "1 for error" built-in: If you call it with a printable argument, it will print this to sys.stderr and use an exit code of 1. This is exactly what you want.
Style issues: The use of "" vs. '' appears inconsistent. I'd stick to one one style unless you really want the other style to avoid escaping something. Also, I'd omit the default "r" for opening a file for reading, and avoid introducing a variable (e) that isn't used.
if __name__ == "__main__":
try:
if options.input_file:
infile = open(options.input_file)
else:
infile = sys.stdin
except IOError:
sys.exit("Cannot open input file %s." % options.input_file)
try:
if options.output_file:
outfile = open(options.output_file, 'w')
else:
outfile = sys.stdout
except IOError:
sys.exit("Cannot open output file %s." % options.output_file)
for line in infile:
# ...
agnul
August 29, 2008, August 29, 2008 07:55, permalink
Me too! All those changes make perfect sense, the only "problem" I see is that having a try block for each open goes straight back to the old days and C, only more verbose. Anyone fancy a flame war on Exceptions vs. Error codes? :-)
Thanks both of you.
luntain
September 1, 2008, September 01, 2008 13:09, permalink
I have extracted the commonality between handling of the input and output into a helper function. I called it safe_open for lack of better idea.
if __name__ == "__main__":
def safe_open(maybe_filename, default, options='r'):
if maybe_filename:
try:
return open(maybe_filename, options)
except IOError:
sys.exit("Cannot open input file %s." % maybe_filename)
return default
infile = safe_open(options.input_file, sys.stdin)
outfile = safe_open(options.output_file, sys.stdout, 'w')
for line in infile:
# ...
Patrick
November 14, 2008, November 14, 2008 20:28, permalink
It would probably be a better idea to group all the code which could result in Very Bad Thingsâ„¢ in one place. So instead of merely opening the file safely and subsequently having to make sure you don't choke while reading/writing or closing the file, just read/write the whole thing safely. This keeps you from doing try:...except every 3 lines when you want to manipulate the file data. It also puts the cost of I/O in one place.
More (maybe even too) verbose? Probably for a short script like this. But if you ever plan on reusing it, I think you'll find it usually pays off later. I didn't test this, but I think it reproduces what you were going for faithfully.
if __name__ == '__main__':
def safe_read(filename, options='r'):
contents = []
try:
fd = open(filename, options)
tmp = fd.readlines()
fd.close()
contents = tmp
except IOError:
print "Can't open file %s." % maybe_filename
return contents
def safe_write(filename, contents, options='w'):
success = False
try:
fd = open(filename, options)
fd.write(contents)
fd.close()
success = True
except IOError:
print "Can't open file %s." % filename
return success
# Try for the file read; if that fails fall back on stdin
for line in safe_read(options.input_file) or sys.stdin:
# ...
# If you want to write something...
some_output = '...'
safe_write(options.output_file or sys.stdout, some_output)
Patrick
November 14, 2008, November 14, 2008 20:34, permalink
I had it slightly wrong above. This is starting to feel like a bit of pythonic abuse, but the meat of the code is still succinct and fairly functional.
Change:
safe_write(options.output_file or sys.stdout, some_output) # => safe_write(options.output_file, some_output) or safe_write(sys.stdout, some_output)
Patrick
November 14, 2008, November 14, 2008 20:38, permalink
Yet more wrong... sys.stdout is a file descriptor, not a filename, and can't be opened. So you'd have to just do a straight write. I suppose it's relatively safe to assume that if stdout writes are failing it's the least of your program's worries.
I'm done. D:
From the useless speculations dept... how would you rewrite this? Those ifs at the beginning are repetitive, the main loop inside the try block feels like a no-no according to PEP-8 and the finally block could try to close a file that was never open in the first place. What do you think?