my first class: Args

[Scott David Daniels]

Peter Kleiweg wrote:
 > 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."
	<http://www.python.org/peps/pep-0008.html>

 > ... 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).
... <keepers>

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

... <keepers>

>         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()