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.
>> 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
> 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.
Received on Thu Jul 20 22:08:36 2006