> 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.
Yeah, it's my mistake. Now i'm reading
http://subversion.tigris.org/hacking.html and i'll ensure not to
repeat this mistake!
> 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.
So, i take no one else have thoroughly reviewed this patch and as
suggested by cmpilato I'll break the changes up into (many) logical
units. Here, i list those logical units. I'll submit the patches for
each logical units seperately.
1) The script should call sys.exit(1) at the end of "except ImportError" block
2) Add a --max-items option
3) --revision could take a range, like the 'svn' client does
4) Lose --url, and add --url-template, which is a URL with replaceable fields
5) Come up with decent default values for options for which
it makes sense to do so:
--svn-path could default to nothing, meaning "look in $PATH"
--rss-file could default to os.path.basename(self.repos_path) \
+ ".rss"
--revision could default to the youngest revision in the repos
6) Validate input before trying to act on it
7) Lose --repos-path, and make it a command-line argument, not an option
> 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...
Ok.
>
> > - 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
--url-template seem to be more appropriate for this option. For each
rss item, we append revision tag to this value and include it in rss
feed. So, this value on its own does not constitute the url (imo).
> 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?
Point. I will include it as another logical unit. I guess, --url
option is more appropriate for this value.
>
> > + 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.
Yeah, good catch, i'll revert it back.
>
> 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.
>
Currently, the script does not seem to support windows system. IMO, we
must give a fresh look and ensure it runs on windows systems. I dont
want to include it as one of the logical unit until i complete
existing ones.
> > + 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()
Ok.
>
> > +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?
Atleast now i'll revert this back. I'll include this support when we
implement full support for windows systems.
> 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...
>
Yeah, i'll incorporate this.
> Or something like that.
>
> Also, watch for consistent use of space-before-style. This script uses
> "foo()", not "foo ()".
Ok.
To conclude, i add following logical units:
8) Retain --url-template for feed item and include --url option for feed url
9) Add following enhancements:
* Consistent use of space-before-style
* define cmd_output() function
* Currently we have 3 rss stuff doing same thing. It's better to
abstract as one
--
Regards,
Bhuvaneswaran
www.symonds.net/~bhuvan/
GPG: 0x7A13E5B0
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jun 16 06:03:07 2006