my first class: Args

[Peter Kleiweg]

I'm still new to Python. All my experience with OO programming
is in a distant past with C++. Now I have written my first class
in Python. The class behaves exactly as I want, but I would like
to get comments about coding style. I'm especially unsure about
how a class should be documented, what to put in, and where.
When to use double quotes, and when single. For instance, the
doc string at the top must be in double quotes, or else the
pydoc search engine won't find the description. Any
recommendation for other mark-up or meta-data I should include?

I won't tell you what the class is about, because that should be
clear from code documentation. If not, I have work to do. Here
it is:


# -*- coding: iso-8859-1 -*-
"""
Handling of arguments: options, arguments, file(s) content iterator

For small scripts that:
- read some command line options
- read some command line positional arguments
- iterate over all lines of some files given on the command line, or stdin if none given
- give usage message if positional arguments are missing
- give usage message if input files are missing and stdin is not redirected
"""

__author__ = 'Peter Kleiweg'
__version__ = '0.1'
__date__ = '2004/08/27'

import os, sys, getopt

class Args:
"""
Instance data:
progname (string) -- name of program
opt (dictionary) -- options with values
infile (string) -- name of current file being processed
lineno (int) -- line number of last line read in current file
linesum (int) -- total of lines read
"""
def __init__(self, usage='Usage: %(progname)s [opt...] [file...]') :
"init, usage string: embed program name as %(progname)s"
self.progname = os.path.basename(sys.argv[0])
self.opt = {}
self.infile = None
self.lineno = 0
self.linesum = 0
self._argv = sys.argv[1:]
self._argc = len(self._argv)
self._usage = usage % {'progname': self.progname}

def __iter__(self):
"iterator set-up"
if self._argc == 0 and sys.stdin.isatty():
self.usage()
if self._argc == 0:
self.infile = '<stdin>'
self._stdin = 1
self._in = sys.stdin
else:
self.infile = self._argv.pop(0)
self._argc -= 1
self._stdin = 0
self._in = open(self.infile, 'r')
return self

def next(self):
"iterator next"
line = self._in.readline()
if line:
self.lineno += 1
self.linesum += 1
return line
self.lineno = -1
self.infile = None
if self._stdin:
raise StopIteration
self._in.close()
if self._argc < 1:
raise StopIteration
self.lineno = 0
self.infile = self._argv.pop(0)
self._argc -= 1
self._in = open(self.infile, 'r')
return self.next()

def warning(self, text):
"print warning message to stderr, possibly with filename and lineno"
if self.lineno > 0:
print >> sys.stderr, '%s:%i: warning: %s' % (self.infile, self.lineno, text)
else:
print >> sys.stderr, '\nWarning %s: %s\n' % (self.progname, text)

def error(self, text):
"print error message to stderr, possibly with filename and lineno, and exit"
if self.lineno > 0:
print >> sys.stderr, '%s:%i: %s' % (self.infile, self.lineno, text)
else:
print >> sys.stderr, '\nError %s: %s\n' % (self.progname, text)
sys.exit(1)

def usage(self):
"print usage message"
print >> sys.stderr, '\n' + self._usage + '\n'
sys.exit(1)

def shift(self):
"pop first of remaining arguments (shift)"
if self._argc < 1:
self.usage()
self._argc -= 1
return self._argv.pop(0)

def pop(self):
"pop last of remaining arguments"
if self._argc < 1:
self.usage()
self._argc -= 1
return self._argv.pop()

def getopt(self, shortopts, longopts=[]):
"get options and merge into dict 'opt'"
options, self._argv = getopt.getopt(self._argv, shortopts, longopts)
self.opt.update(dict(options))
self._argc = len(self._argv)


if __name__ == '__main__':

a = Args('Usage: %(progname)s [-a value] [-b value] [-c] word [file...]')

a.opt['-a'] = 'option a' # set some default option values
a.opt['-b'] = 'option b' #
a.getopt('a:b:c') # get user supplied option values

word = a.shift() # get the first of the remaining arguments
# use a.pop() to get the last instead

for line in a: # iterate over the contents of all remaining arguments (filenames)
if a.lineno == 1:
print 'starting new file:', a.infile
a.warning(line.rstrip())

print 'Options:', a.opt
print 'Word:', word
print 'Total number of lines:', a.linesum

print sys.argv # unchanged

a.warning('warn 1') # print a warning
a.error('error') # print an error message and exit
a.warning('warn 2') # this won't show

 

[Tom B.]

Looks just beautiful, I always leave docstrings out because of laziness. As
for quotes you don't need to put "" if you have nothing to say and python
lets you use 'single', "double", or even """the 'magnificent' triple "quote"
""" in code.

Tom

 

[Scott David Daniels]

> I'm still new to Python. All my experience with OO programming
> is in a distant past with C++. Now I have written my first class
> in Python. The class behaves exactly as I want, but I would like
> to get comments about coding style.

I like: "look at a coding standard (PEP 8), then vary for clarity."

> ... when to use double quotes, and when single.

I generally prefer single a lot of places since typefaces sometimes make
it hard to tell the difference between a pair of singles and a double.

Here is a lot of blue pencil. Don't consider that a rip of your code.
Don't consider me correct, see each change and decide for yourself
which way is clearer. One thing I use here several places is the
"try and fail, don't ask permission" -- a pythonic style.

I won't tell you what the class is about,

Ah, but everyone has a context in which they read code.

"""
Handling of arguments: options, arguments, file(s) content iterator

^^ Try "Demonstrate use of arguments..." (a verb helps).

class Args:
"""

^^ Here put a line about what the class is about:
Source provision for numbered lines with (possibly) multiple inputs.

self._argv = sys.argv[1:]
self._argc = len(self._argv)

^^ I'd forego _argc altogether and use len(self._argv) where needed.

self._usage = usage % {'progname': self.progname}

^^ I'd wait to do this in the usage method -- no need for it normally
self._usage = usage

def __iter__(self):
"iterator set-up"

^^ useless docstring -- presume the reader knows python

if self._argc == 0 and sys.stdin.isatty():
self.usage()
if self._argc == 0:
self.infile = '<stdin>'
self._stdin = 1
self._in = sys.stdin
else:
self.infile = self._argv.pop(0)
self._argc -= 1
self._stdin = 0
self._in = open(self.infile, 'r')
return self

^^
try:
self.infile = self._argv.pop(0)
except IndexError:
if sys.stdin.isatty():
self.usage() # Doesn't return
else:
self.infile = '<stdin>'
self._stdin = True
self._in = sys.stdin
else:
self._stdin = False
self._in = open(self.infile, 'r')
return self

def next(self):
"iterator next"

^^ again -- skip the comment unless you say more than the language does.
Maybe: "get another line, possibly going to another file for it"

line = self._in.readline()
if line:
self.lineno += 1
self.linesum += 1
return line
self.lineno = -1
self.infile = None
if self._stdin:
raise StopIteration
self._in.close()
if self._argc < 1:
raise StopIteration
self.lineno = 0
self.infile = self._argv.pop(0)
self._argc -= 1
self._in = open(self.infile, 'r')
return self.next()

^^ Loop rather than recur unless you have a good reason.

while True:
line = self._in.readline()
if line:
self.lineno += 1
self.linesum += 1
return line

# Look for a source of more lines
if self._stdin:
assert not self._argv # stdin is not part of a list
break

self._in.close()
try:
self.infile = self._argv.pop(0)
except IndexError:
break # No more files
self.lineno = 0
self._in = open(self.infile, 'r')

self.lineno = -1
self.infile = None
raise StopIteration

def warning(self, text):
"print warning message to stderr, possibly with filename and lineno"
if self.lineno > 0:
print >> sys.stderr, '%s:%i: warning: %s' % (self.infile, self.lineno, text)
else:
print >> sys.stderr, '\nWarning %s: %s\n' % (self.progname, text)

^^ these lines look a bit long. Consider:
if self.lineno > 0:
print >>sys.stderr, '%s:%i: warning: %s' % (
self.infile, self.lineno, text)
else:
print >>sys.stderr, '\nWarning %s: %s\n' % (
self.progname, text)

or even consider:
def error(self, text, fatal=True):
if fatal:
style = 'Error'
else:
style = 'Warning'
if self.lineno > 0:
print >>sys.stderr, '%s:%i: %s:' % (
self.infile, self.lineno, style),
else:
print >>sys.stderr, '\n%s %s:' % (style, self.progname),
print >>sys.stderr, text
if fatal:
sys.exit(1) # or even: raise SystemExit

Where warnings look like: obj.error(msg, fatal=False)

def usage(self):
"print usage message"
print >> sys.stderr, '\n' + self._usage + '\n'
sys.exit(1)

^^ As mentioned above:

def usage(self):
"print usage message and leave"
print >> sys.stderr,
print >> sys.stderr, self._usage % {'progname': self.progname}
print >> sys.stderr,
sys.exit(1)

def shift(self):
"pop first of remaining arguments (shift)"
if self._argc < 1:
self.usage()
self._argc -= 1
return self._argv.pop(0)

^^ becomes:
def shift(self):
"pop first of remaining arguments (shift)"
try:
return self._argv.pop(0)
except IndexError:
self.usage()

def pop(self):
"pop last of remaining arguments"
if self._argc < 1:
self.usage()
self._argc -= 1
return self._argv.pop()

^^ becomes:
def pop(self):
"pop last of remaining arguments"
try:
return self._argv.pop()
except IndexError:
self.usage()

def getopt(self, shortopts, longopts=[]):
"get options and merge into dict 'opt'"
options, self._argv = getopt.getopt(self._argv, shortopts, longopts)
self.opt.update(dict(options))

self._argc = len(self._argv)

^^ As I've said, I'd drop this one

if __name__ == '__main__':
> ...

^^ also nicer to be able to test from included version:

def demo():
...

if __name__ == '__main__':
demo()

 

[Peter Kleiweg]

Thanks for your comments. Especially the thing about using
try/except is very useful. I knew about it, but just didn't
"see" I could use it. Something I have to get used to.

More comments below.

^^ useless docstring -- presume the reader knows python
^^
try:
self.infile = self._argv.pop(0)
except IndexError:
if sys.stdin.isatty():
self.usage() # Doesn't return
else:
self.infile = '<stdin>'
self._stdin = True
self._in = sys.stdin
else:
self._stdin = False
self._in = open(self.infile, 'r')
return self

This is the only instance I didn't follow your recommendation to
use try/except. It is a binary choice: do we use files given on
the command line or stdin? Neither is more natural than the
other. Also, it splits up lines of code that belong together.

Here is my re-write:

def __iter__(self):
"iterator: set-up"
if self._argv:
self.infile = self._argv.pop(0)
self._in = open(self.infile, 'r')
self._stdin = False
else:
if sys.stdin.isatty():
self.usage() # Doesn't return
self.infile = '<stdin>'
self._in = sys.stdin
self._stdin = True
return self

The exceptional case is there be no input, and the simplest test
here seems to be a simple if-statement.

^^ Loop rather than recur unless you have a good reason.

Your solution looks much better.

^^ also nicer to be able to test from included version:

def demo():
...

if __name__ == '__main__':
demo()

That doesn't seem very useful. It depends on command line
arguments. If they are missing, it does a sys.exit(), probably
not what you want unless you run the module stand-alone.

 

[Scott David Daniels]

Peter Kleiweg wrote:

....

This is the only instance I didn't follow your recommendation to
use try/except. It is a binary choice: do we use files given on
the command line or stdin? Neither is more natural than the
other. Also, it splits up lines of code that belong together.

Here is my re-write:

def __iter__(self):
"iterator: set-up"
if self._argv:
self.infile = self._argv.pop(0)
self._in = open(self.infile, 'r')
self._stdin = False
else:
if sys.stdin.isatty():
self.usage() # Doesn't return
self.infile = '<stdin>'
self._in = sys.stdin
self._stdin = True
return self

The exceptional case is there be no input, and the simplest test
here seems to be a simple if-statement.

Perfectly fine. I probably lean on the "don't look before you leap"
a bit too heavily. Remember, writing code is like writing in your
native language; it is more important to be clear than to match a
style. Notice that you have a bit more structure now than originally.
A single top-level conditional is simpler to read; less needs to be
read to understand what is going on.

That doesn't seem very useful. It depends on command line
arguments. If they are missing, it does a sys.exit(), probably
not what you want unless you run the module stand-alone.

I missed the sys.argv -- here is a sketch of a slightly better rewrite:

> class Args:
> """..."""
> def __init__(self,usage='Usage: %(progname)s [opt...] [file...]'):
> "init, usage string: embed program name as %(progname)s"
> self.progname = os.path.basename(sys.argv[0])
> ...
> self._argv = sys.argv[1:]

class Args:
"""..."""
def __init__(self, progname, args,
usage='Usage: %(progname)s [opt...] [file...]'):
"usage string: embed program name as %(progname)s"
self.progname = progname
self._argv = args
...

> if __name__ == '__main__':
> ...

def demo(progname, args):
a = Args(progname, args,
'Usage: %(progname)s [-a value] [-b value] [-c] word [file...]')
...

if __name__ == '__main__':
demo(sys.argv[0], sys.argv[1:])


---

In idle, the exit can be stopped (so it is not a huge worry). If
nothing else you can do:
try: demo('name', ['abc', 'def']}
except SystemExit: pass

Cutting into sys.argv is, however, a bit tougher usually.


-Scott David Daniels
(e-mail address removed)

 

[Peter Kleiweg]

Scott David Daniels schreef:

I missed the sys.argv -- here is a sketch of a slightly better rewrite:

class Args:
"""..."""
def __init__(self,usage='Usage: %(progname)s [opt...] [file...]'):
"init, usage string: embed program name as %(progname)s"
self.progname = os.path.basename(sys.argv[0])
...
self._argv = sys.argv[1:]

class Args:
"""..."""
def __init__(self, progname, args,
usage='Usage: %(progname)s [opt...] [file...]'):
"usage string: embed program name as %(progname)s"
self.progname = progname
self._argv = args
...

if __name__ == '__main__':
...

def demo(progname, args):
a = Args(progname, args,
'Usage: %(progname)s [-a value] [-b value] [-c] word [file...]')
...

if __name__ == '__main__':
demo(sys.argv[0], sys.argv[1:])

This goes against the purpose of the class: to take care of as
much of the overhead of script writing as possible. So I can
simply do this:

import args

a = Args()
for line in a:
do_something_with(line)

Or with some extras:

import args

a = Args()
for line in a:
try:
do_something_with(line)
except SomeError:
a.error("something wrong")

In the last example, I would get filename and line number of the
input were the error occured, in the format understood by the
'make' parser of Emacs.

 

[Scott David Daniels]

Scott David Daniels schreef:

class Args: ...
def __init__(self,usage=<some text>):
...
self._argv = sys.argv[1:]

>>if __name__ == '__main__':
>> <lotsa code> -->
class Args: ...
def __init__(self, progname, args, usage=<some text>):

def demo(progname, args):
a = Args(progname, args,
'Usage: %(progname)s [-a value] [-b value] [-c] word [file...]')
<lotsa code 'much like' above>

if __name__ == '__main__':
demo(sys.argv[0], sys.argv[1:])

>
> This goes against the purpose of the class: to take care of as
> much of the overhead of script writing as possible. So I can
> simply do this:

^^ By the way, this goes at the top of your module comment -- it
lets users understand in 3 (or 6 if you add the example) lines.

>
> import args
>
> a = args.Args()
> for line in a: ...

I am not saying you are wrong, exactly, but I will explain why I
prefer my style. In my style, I increase the chance that I can
reuse programs without changing (or even really reading) them.
This allows something else to get "in the way" when you decide
to build another application from some you already have.

For example:

import sys, app1, app2

progname = sys.argv[0]
break = sys.argv.index(1, ';')
app1.main(progname + 'part1', sys.argv[1 : sep])
app2.main(progname + 'part2', sys.argv[sep :])


Perhaps a compromise between the two styles would be closer to
your aesthetic, where the call is:

a = args.Args(sys.argv)
for line in a: ...

More work on combining, but less work on using. At any rate it has
been fun blathering about style.


--Scott David Daniels
(e-mail address removed)