Re: [PATCH] Atom 1.0 feeds for subversion

On Tue, Jul 18, 2006 at 02:52:18PM +0530, Bhuvaneswaran Arumugam wrote:
> Hello,
>
> Based on the discussion [1] i have written a new python script to
> generate Atom 1.0 feeds for commit information. It uses xml dom library
> to generate the feeds.
>

Nice! Here's another quick review, though not so much focussed on the
implementation, since my Python's not that hot.

Some quick notes:

* The usage message could be a lot clearer. I couldn't determine what
  the effect of some of the switches would be without using them.

* The output Atom file declares the XHTML namespace, but doesn't use it.

* There's a fair bit of unnecessary trailing whitespace in the file.

> + -f, --atom-file=PATH Store the ATOM feed in the file located at PATH,
> + which will be created if it doesn't already exist.
> + If not provided, the script will store the feed in
> + the current working directory, in a file named
> + REPOS_NAME.atom (where REPOS_NAME is the basename
> + of the REPOS_PATH command-line argument).
> +

In my testing, the script generated a file called '.atom', not
REPOS_NAME.atom.

> + -u, --item-url=URL Use URL as the basis for generating ATOM item links.
> +

The generated URL for each item is ITEM_URL?rev=N. That's a bit odd
(it assumes a particular URL scheme), but if we decide to keep it,
we should document it here.

> + -U, --feed-url=URL Use URL as the global ATOM feed link.
> +

Atom feeds require both a feed URL and item URL - or perhaps you can drop
the feed URL if every item has a URL, I'm not exactly sure. Whichever it
is, there's no point in allowing the user to be able to generate invalid
Atom feeds (with URL's such as 'None', as currently happens).

* The generated feeds don't pass the feed validator[1], since they lack an
  atom:link entry with rel="self".

Finally, I needed to apply the attached patch to get this to work
at all. I'm assuming you're using something outside of the standard
Python libraries for XML pretty-printing, so I just disabled that bit.
I think you should also be using a different method to get at the
DOMImplementation object (see the patch).

(Okay, I don't think I really needed to remove the StringIO import,
but it didn't seem to be used either).

Regards,
Malcolm

[1] http://feedvalidator.org/, passing of which should be a prerequisite to
    checking this in, I feel.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org