Bhuvaneswaran Arumugam wrote:
...
Please try to avoid cc-ing svn@s.t.o - it creates needless duplicate
messages.
>> - if(childNode.nodeName == 'author'):
>> - self.feed.insertBefore(item, childNode.nextSibling)
>> - elif(childNode.nodeName == 'entry'):
>> + if childNode.nodeName == 'entry':
>> + if total == 0:
>> + self.feed.insertBefore(item, childNode)
>> + total += 1
>> total += 1
>
> Weird! We increment the "total" twice when the value is 0. Result: We
> manage to store max_items-1 items in the feed file. I bet, you intend to
> remove the increment statement which resides inside the "if total == 0:"
> loop :)
No, I meant exactly what I committed.
You should count both the <entry> node you have just seen, and the one
you are inserting. 1 + 1 = 2.
Or at least, that's the idea. It doesn't work, though, because it seems
that it is not safe to iterate over a childNodes collection whilst
changing it.
The old code just happened to work, because it suffered from two bugs
which had precisely opposing effects, cancelling each other out:
(i): off by one error, fails to count newly inserted node
(ii): changes childNodes whilst iterating over it, leading to
unexpected behaviour.
I fixed (i), and now (ii) is no longer accidentally cancelled out.
>> -- if (total > self.max_items):
>> - self.feed.removeChild(self.feed.lastChild)
>> + if total > self.max_items:
>> + self.feed.removeChild(childNode)
>
> Here again, why do we wish to include the above "if" condition for every
> childNode ? We call this function for every revision, so it's good to
> place it outside the "for" loop. Moreover, in the current state it'll
> not remove multiple nodes (if you intend to), since we increment the
> "total" one at a time.
No, it is not good to place it outside the for loop; it would not then
be able to remove multiple nodes.
I believe it *will* remove multiple nodes if needed - I don't understand
your explanation of why you think it won't.
Max.
Received on Sat Aug 5 21:08:02 2006