Re: svn2rss.py patch

Bhuvaneswaran wrote:
> I guess I have addressed all the issues/enhancements suggested by
> cmpilato. I have also fine tuned the error handling mechanism.
> Herewith, I've attached the patch. Please verify and apply. Thank you!

Wow. I tried to review this patch, but it's just too much churn for me to
digest at once. For one thing, you didn't provide a log message, which
means I don't have that helpful hint alongside as I review. Secondly, it
appears you combined more than a handful of changes that each deserve their
own distinct patch/review/commit into one giant patch.

I'm going to give some fly-by thoughts on the patch itself, but a) do not
consider them to represent a thorough review, and b) if you wanna take
another crack at this, please break the changes up into (many) logical units.

> for opt, arg in opts:
> if opt in ("-h", "--help"):
> usage(sys.stdout)
> sys.exit(0)
> - elif opt in ("-P", "--svn-path"):
> + elif opt in ("-p", "--svn-path"):
> svn_path = arg
> elif opt in ("-r", "--revision"):
> commit_rev = arg
> - elif opt in ("-p", "--repos-path"):
> - repos_path = arg

This change is pretty much guaranteed to confuse folks that upgrade from an
older version of the script. That said, I doubt there are swarming masses
that fit that description...

> - elif opt in ("-u", "--url"):
> + elif opt in ("-u", "--url-template"):
> url = arg

You changed this option name, but the behavior didn't really change at all.
 So ... why the change at all? Also, I noticed that there are actually two
URLs in play in this script -- one that is associated with the feed name,
and one that's associated with the individual items. Why is only one of
those exposed via the options?

> + def __init__(self, svn_path, revision, repos_path, url, rss_file, max_items):
> +
> + self.project_url = "www.foo.com/svn"
> + if (svn_path == None):
> + self.svn_path = self.get_svn_path()
> + else:
> + self.svn_path = svn_path
> +
> + self.svnlook = os.path.join(self.svn_path, "svnlook")
> +
> + if (not os.path.exists (self.svnlook)):
> + print >> sys.stderr, "svn2rss.py: 'svnlook' command does not exist in %s directory" % self.svn_path
> + print >> sys.stderr, " Please specify the correct path"
> + sys.exit(1)

get_svn_path() does the same kind of sanity check -- I'm thinking it doesn't
need to, since its caller will be checking the existence of the svnlook
program anyway.

Another take on this, though -- not sure how portable this script is
intended to be, but poking around in $PATH and looking for explicitly named
programs is almost sure to fall over on Windows systems.

> + def get_youngest_revision (self):
> + """ Get youngest revision of the svn repository """
> +
> + try:
> + cmd = self.svnlook + " youngest " + self.repos_path
> + out, x, y = popen2.popen3(cmd)
> + cmd_out = out.readlines()

I think I counted three (3) places where these types of call are done.
Suggest a new function:

   def cmd_output(cmd):
      out, unused1, unused2 = popen2.popen3(cmd)
      return out.readlines()

> +try:
> + # remove / or \ in the end to pass it to os.path.basename function
> + repos_path = re.sub ('\/\s*$', '', repos_path) # for linux fs
> + repos_path = re.sub ('\\\s*$', '', repos_path) # for win fs

Aha. So there is *some* belief that this should run on Windows. Well,
given what I've seen so far, I'm kinda doubting it. I mean, svnlook on
Windows is gonna be called 'svnlook.exe', right?

> + # if revision doesnt contain ":"
> + if (not re.compile(":").search(commit_rev)):
> + svn2rss = SVN2RSS(svn_path, commit_rev, repos_path, url, rss_file, max_items)
> + rss = svn2rss.rss
> + svn2rss.pickle()
> + fh = open (svn2rss.rss_file, "w")
> + rss.write_xml(fh)
> + fh.close()
> + # 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)
> + # if the revision range is m:n (m > n)
> + if (end <= start):
> + while (end <= start):
> + commit_rev = str(end)
> + svn2rss = SVN2RSS(svn_path, commit_rev, repos_path, url, rss_file, max_items)
> + end = end + 1;
> +
> + rss = svn2rss.rss
> + svn2rss.pickle()
> +
> + fh = open (svn2rss.rss_file, "w")
> + rss.write_xml(fh)
> + fh.close()
> + # if the revision range is n:m (m > n)
> + else:
> + while (end >= start):
> + commit_rev = str(end)
> + svn2rss = SVN2RSS(svn_path, commit_rev, repos_path, url, rss_file, max_items)
> + end = end - 1;
> +
> + rss = svn2rss.rss
> + svn2rss.pickle()
> +
> + fh = open (svn2rss.rss_file, "w")
> + rss.write_xml(fh)
> + fh.close()

Ick. You've got three logic forks here doing the exact same thing. Either
abstract out the real RSS stuff, or change the three forks so they their
only purposes is to build a list of revisions to iterate over:

   if (not re.compile(":").search(commit_rev)):
       revs = [commit_rev]
   else:
       start, end = string.split(commit_rev, ':')
       if start < end:
           revs = range(start, end + 1, 1)
       else:
           revs = range(start, end - 1, -1)
   for rev in revs:
       # do the real RSS stuff here...

Or something like that.

Also, watch for consistent use of space-before-style. This script uses
"foo()", not "foo ()".

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