Re: [PATCH] Atom 1.0 feeds for subversion

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

Ah, I *see*. In that case we must merge the code into a unified feed
producing script. To do otherwise is a severe maintenance headache.

So, that's what I have done. Please review the latest version of
svn2rss.py, in which I think I have incorporated all your changes,
though in substantially different form.

>>>> TODO: Implement a scheme to insert the items to the file if it already
>>>> exists (using pickle?) and make use of --max-items option.

Implementing insertion turned out to be really easy, so I just did it.

For now, item removal is NOT implemented, and needs to be. It shouldn't
be too difficult, but I do not know DOM very well, so I left it for now.

>>> + 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.

It seems very weird to have two functions which are only ever invoked
one after the other. Therefore, I have merged them.

>>> + 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.

Actually, it was still in your second version of the patch. I dropped it.

>>> + 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?

The tags are required, yes, but they can be local variables of the
function. They do not need to be member variables of the Svn2Atom instance.

>>> + 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.

Is it required to be so for feed readers to work properly? Otherwise, I
think it might make more sense to use the revision date.

>> desc_lines.append('')
>> item_desc = "\n".join(desc_lines)
>
> Yes, i have incorporated this change.

Actually, I meant, use this to replace the use of individual variables
entirely - have a look at my version to see what I meant.

>>> + 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.

The point was to avoid creating a range at all - your code really just
did what range() does, but manually.

Since this is pre-existing code in svn2rss.py, we can leave it and tidy
it up later.

Max.