Re: [PATCH] svn2rss.py: Improve error handling and add --max-itemsoption

Bhuvaneswaran Arumugam wrote:
> Hello,
>
> I have attached a patch for svn2rss.py to improve error handling and add
> --max-items option.
>
> [[[
> Improve error handling and add --max-items option
>
> Patch by: Bhuvaneswaran <bhuvan@collab.net>
> Suggested by: cmpilato
>
> * svn2rss.py
> add sys.exit(1) in 'except ImportError' block
> Add --max-items option
> (__init__): If max_items is not set, set it to 20
> ]]]

Cool. You are still tackling two different issues in one change, but I know
how hard it is to resist piggybacking a one-liner onto something else.
Decent log message, though I suppose all the items listed are technically
part of __init__.

> Index: svn2rss.py

Note for the future -- always generate your patches from the top of the
Subversion tree.

> ===================================================================
> --- svn2rss.py (revision 20142)
> +++ svn2rss.py (working copy)
> @@ -8,6 +8,7 @@
> -p | --repos-path= svn repository to generate RSS 2.0 feed
> -u | --url= link to appear in the rss item
> -f | --rss-file= filename to store the rss feed
> + -m | --max-items= maximum items to store in the rss feed
>
> Generates a RSS 2.0 file containing commit information. Once the
> maximum number of items is reached, older elements are removed. The
> @@ -33,6 +34,7 @@
> print >> sys.stderr, "PyRSS2Gen can be downloaded from:"
> print >> sys.stderr, "http://www.dalkescientific.com/Python/PyRSS2Gen.html"
> print >> sys.stderr, ""
> + sys.exit(1)

Here's that change that technically should be in its own commit. I'm going
to go ahead and commit that part of it.

> @@ -41,16 +43,19 @@
> usage(sys.stderr)
> sys.exit(2)
> try:
> - opts, args = getopt.gnu_getopt(sys.argv[1:],"hP:r:p:u:f:", [
> + opts, args = getopt.gnu_getopt(sys.argv[1:],"hP:r:p:u:f:m:", [
> "help", "svn-path=",
> "revision=",
> "repos-path=", "url=",
> - "rss-file="])
> + "rss-file=",
> + "max-items="])
> except getopt.GetoptError, msg:
> print >> sys.stderr, msg
> usage(sys.stderr)
> sys.exit(2)
>
> +max_items = None
> +

Now, this is odd. Why not go ahead and set max_items = 20?

> for opt, arg in opts:
> if opt in ("-h", "--help"):
> usage(sys.stdout)
> @@ -65,15 +70,22 @@
> url = arg
> elif opt in ("-f", "--rss-file"):
> rss_file = arg
> + elif opt in ("-m", "--max-items"):
> + max_items = arg

Here, you should validate the input a bit:

   try:
       max_items = arg
   except ValueError, msg:
       usage_and_exit("Invalid value '%s' for --max-items." % (arg))
   if max_items < 1:
       usage_and_exit("Value for --max-items must be a positive integer.")

> class SVN2RSS:
> - def __init__(self, svn_path, revision, repos_path, url, rss_file):
> - self.max_items = 20
> + def __init__(self, svn_path, revision, repos_path, url, rss_file, max_items):
> self.svn_path = svn_path
> self.revision = revision
> self.repos_path = repos_path
> self.url = url
> self.rss_file = rss_file
> +
> + if (max_items == None):
> + self.max_items = 20
> + else:
> + self.max_items = int(max_items)

Ah, so you made the *class* have a notion of a default. You don't permit
max_items to be an optional input, and you document that None is allowed, so
I'd recommend doing as I said about (setting the default outside the class),
and here just a simple:

    self.max_items = max_items

-- 
C. Michael Pilato <cmpilato@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand