Re: [PATCH] Atom 1.0 feeds for subversion

Max, thanks for your review! Please find attached the revised patch and
see my comments below.

> > TODO: Implement a scheme to insert the items to the file if it already
> > exists (using pickle?) and make use of --max-items option.
>
> If the file *only* contains Subversion commit entries, then all the data
> required to reproduce old items is in the Subversion repository, so
> there should be no need to pickle, or re-read the file.

Yep. But, the Atom feed should be updated with recent revisions on
periodic basis. For example, when the feed contains revision 5 .. 1 and
if we run the script, the feed should be updated to have revisions 6,
5 ... 1. Please refer svn2rss.py for more details.

This script is almost inherited from svn2rss.py. So, all the
corrections/changes we do here must be done in that script as well.

> 'Python xml.dom module' explains what you mean more clearly.
>
> Also, missing '.' at end of sentences.

[[
Patch by: Bhuvaneswaran Arumugam <bhuvan@collab.net>
Tweaked by: malcolm
Reviewed by: malcolm
             maxb

* contrib/hook-scripts/svn2atom.py
  New script to generate Atom 1.0 feeds for commit information. It uses
  python xml.dom module to generate the feeds.

* www/tools_contrib.html
  Include the documentation for svn2atom.py script.
]]

> ATOM or Atom? Find out which is officially correct, and use it exclusively.

In all reference sites (ex: [1], [2]) they use Atom and never use ATOM.
So, i'm assuming it is officially correct and use it.

> Explain what happens if the file already exists. It will be overwritten?

Ok. I have addressed it.

> If keeping of old items isn't implemented yet, then the above comment is
> wrong.
>
> If you don't want to implement keeping of old items in the initial
> version, that is fine, but the documentation must reflect that. To put
> it another way: missing features are acceptable. Bugs are not.

Yeah, for now, i have not implemented the method to keep old items.

> > + -u, --item-url=URL Use URL as the basis for generating ATOM item links.
>
> It's unclear what you are supposed to use this for without reading the code.
>
> > + -U, --feed-url=URL Use URL as the global ATOM feed link.
>
> Ditto.

I have added more description.

> > + -P, --svn-path=DIR Look in DIR for the svnlook binary. If not provided,
> > + the script will run "svnlook" via a typical $PATH hunt.
>
> Say: "svnlook" must be on the PATH.
> Not: the script will run "svnlook" via a typical $PATH hunt.
>
> because the old version implies that the script is hunting though PATH
> itself.

Yeah, i have corrected it.

>
> > +"""
> > +
> > +import sys
> > +
> > +# All clear on the custom module checks. Import some standard stuff.
>
> Wrong comment. There were no custom module checks. Also "Import some
> standard stuff." is entirely self evident. Delete the comment.

I have corrected it.

> Please use PEP 8 style <http://www.python.org/dev/peps/pep-0008/>. In
> this case, that means each of the above imports gets a separate line.

Thanks for the link. Now, each import statement gets a separate line!

> > + stream = errmsg is not None and sys.stderr or sys.stdout
>
> This feels too unreadable for me, I'd prefer an explicit "if" statement.

I have incorporated this change.

> No. What if someone wants to put a svn:// URL in there? Or an ftp://
> one? Or even a relative URL?
>
> I think this check should be removed.

The URL we are talking here is an Atom feed URL. It could be an http://,
https:// or file:// based URL. This value cannot be a svn:// or ftp://
based URL. So, we need not remove this check.

> > +class SVN2ATOM:
>
> PEP 8 ==> Svn2Atom

I have corrected it.

> > + self.init_output()
> > + self.add_header()
>
> Why are these not one combined function?

We use it only here. So, IMO one combined function is unnecessary.

> > + self.feed.appendChild(doc.createTextNode("\n "))
> > + self.feed.setAttribute("xml:lang", "en")
>
> How can you be certain of this? Better to leave it blank than set it to
> a possibly incorrect value.

I have corrected it.

> > + self.title.appendChild(doc.createTextNode("%s SVN Commit's Feed" % \
>
> I would prefer 'Subversion' in place of 'SVN'.
> Incorrect use of apostrophe. Remove.
> Do not use redundant backslashes.

I have corrected it.

> > + self.id.appendChild(doc.createTextNode("%s" % self.feed_url or ""))
>
> What's with the 'or ""' ?

It's intended to store "" in case self.feed_url is None. I have
corrected it.

> If you are going to require Python 2.3, then you must state that at the
> top of the script.

Now, I have stated it at the top of the script.

> > + self.author.appendChild(self.aname)
> > + self.aname.appendChild(doc.createTextNode("subversion"))
>
> Is it really necessary for all these nodes to be member variables?

These entries are necessary for an Atom feed. If i get you wrong, can
you please elaborate?

> > + self.item_url + "?rev=%d" % self.revision or ""
>
> Hardcoding of ?rev= is not reasonable. Arrange for there to be a marker
> in item_url which you replace.

As i mentioned earlier, this script is almost inherited from svn2rss.py
script. I should come up with such marker for both these scripts and
i'll take it separately.

> > + doc.createTextNode("%s" % self.format_date(datetime.datetime.now())))
>
> Now? Shouldn't the time be taken from the revision's svn:date?

I guess it should be the entry published/updated date instead of
revision date.

> > + cmd = "%s info -r %s %s" % (self.svnlook_cmd, self.revision, self.repos_path)
>
> Use list style commands, as you do later in main(), to avoid shell
> metacharacter issues.

Yes, i have incorporated this change.

> > + out, x, y = popen2.popen3(cmd)
>
> x? y? Yuck, use clear variable names.

Yes, i'm using clear variable names now!

> But don't the lines already have a newline attached?

Yes, they do have a newline attached. So, i have corrected it.

> Weird capitalization. Local variables should be lower-case.

Yes, i have corrected it.

> > + changed_files = changed_files + item
>
> +=

Yes, i have incorporated this change. Thanks to madan for clarifying
this comment :)

> desc_lines.append('')
> item_desc = "\n".join(desc_lines)

Yes, i have incorporated this change.

> > + atom_file = os.path.basename(repos_path) + ".atom"
>
> Is this a normal file extension convention for atom files?

Yes, as per [1] this is the normal file extension convention for atom
files.

> > + if (commit_rev == None):
>
> Un-Pythonic parentheses.
>
> It is conventional to test "is None", not "== None".

Yes, i have incorporated this change.

> > + except ValueError, msg:
> > + usage_and_exit("svn2atom.py: Invalid value '%s' for --revision." \
> > + % (commit_rev))
>
> What's this supposed to catch, exactly? I cannot think of any
> circumstances in which a ValueError would be raised here.

Earlier, the 'except' was not in correct loop. I have corrected it and
it is intended to handle invalid revision values (ex: 1:a).

> > + revisions = range(start, end + 1)[-max_items:]
>
> range() creates an explicit list. Nasty things will happen if the user
> specifies a large revision range here. Rewrite to not use range(), by
> simply using the start and end variables to control a loop variable
> directly.

I have incorporated this change.

[1]
http://en.wikipedia.org/wiki/Atom_(standard)#Atom_1.0_and_IETF_Standardization
[2] http://www.atomenabled.org/developers/syndication/

-- 
Regards,
Bhuvaneswaran