Re: [PATCH] svn2rss.py: --revision takes a range (m:n)

Bhuvaneswaran Arumugam wrote:
> Hello,
>
> I have attached a patch for svn2rss.py python script. The patch contains
> following fixes and they are related to each other:
>
> 1) --revision takes a range (m:n), like the 'svn' client does
> 2) if the revision is not set, it is set to the youngest one
>
> [[[
> --revision takes a range (m:n), like the 'svn' client does and default
> value is set to the youngest one
>
> * svn2rss.py
> (import): import re, string

I'd leave the "(import): " part off of this line. You aren't really
modifying a symbol named "import" ... this is the equivalent of adding a
#include in C code, so:

* contrib/hook-scripts/svn2rss.py
  Import 're' and 'string' modules.
  (__init__): Add 'try' block and handle ValueError for revision. Here,
    if the revision is not set, set it to the youngest one. Handle one
    and range of revisions. Finally, for each revision, add the rss feed
    to the file.
  (SVN2RSS.__init__): Pop the rss items from the feed, while length of
    rss feed >= max items

> Index: contrib/hook-scripts/svn2rss.py
> ===================================================================
> --- contrib/hook-scripts/svn2rss.py (revision 20157)
> +++ contrib/hook-scripts/svn2rss.py (working copy)
> @@ -34,7 +34,7 @@
> sys.exit(1)
>
> # All clear on the custom module checks. Import some standard stuff.
> -import getopt, os, popen2, pickle, datetime
> +import getopt, os, popen2, pickle, datetime, re, string

Scripts written for modern versions of Python almost never need 'string'
imported explicitly. Details below.

> @@ -152,8 +153,10 @@
> f = open(self.pickle_file, "r")
> rss = pickle.load(f)
> f.close()
> - if len(rss.items) == self.max_items :
> + length = len(rss.items)
> + while (length >= self.max_items):
> rss.items.pop()
> + length = length - 1
> rss.items.insert(0, self.rss_item)

I wonder if this couldn't be done a little more cleverly. Something like
(NOTE: untested code):

            rss = pickle.load(open(self.pickle_file, "r"))
            rss.items.insert(0, self.rss_item)
            if len(rss.items) > self.max_items:
                del(rss.items[self.max_items:])

> +try:
> + if (commit_rev is None):
> + cmd = "svnlook youngest " + repos_path
> + out, x, y = popen2.popen3(cmd)
> + cmd_out = out.readlines()
> + revisions = [int(cmd_out[0])]
> + out.close()
> + x.close()
> + y.close()
> + elif (not re.compile(":").search(commit_rev)):
> + revisions = [int(commit_rev)]
> + # if revision contains ":", split and add the rss items for all
> + # revisions mentioned in the range
> + else:
> + start, end = string.split (commit_rev, ':')
> + start = int(start)
> + end = int(end)

There are a number of problems with new code. First, it uses a two-space
indentation, whereas the local policy is four-space indentation. Secondly,
there are instances of function-space-paren, where local policy dictate no
spaces between function name and parenthesis.

You're using a regexp compilation and search for a single character (":"),
when you could just do:

    elif commit_rev.find(":") == -1:

Here's where your string module gets used, but a) you could just import that
module in the one logic fork that needs it:

    else:
        import string
        start, end = string.split(commit_rev, ':')

or, you could avoid the import altogether:

    else:
        start, end = commit_rev.split(':')

In fact, because it's okay to split on a character that isn't in the string,
you can simplify the whole of the revision input handling like so:

    rev_range = commit_rev.split(':')
    len_rev_range = len(rev_range)
    if len_rev_range == 0:
        # TODO: do the 'svnlook youngest' thang
    elif len_rev_range == 1:
        revisions = rev_range
    elif len_rev_range == 2:
        # The rss feed is a stack. Generate the rss item from 'end'
        # revision, so it is moved down and 'start' revision remains as
        # first rss item
        if (start < end):
            revisions = range(end, start - 1, -1)
        else:
            revisions = range(end , start + 1, 1)
    else:
        usage_and_exit("Revision specification '%s' is invalid." \
                       % (commit_rev))

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