Bhuvaneswaran Arumugam wrote:
>>>> - 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.
>
> Nope! Here, the loop is executed separately for the item we insert using
> "insertBefore" function and it does not bother to iterate over a
> childNodes collection whilst changing it. In current state, when we run
> the script as below, it inserts only 9 items instead of 10 items.
>
> $ ./svn2feed.py -F atom -u .. -U .. -r1:10 /path/to/repos/ -m 10
> # remove the feed and pickle files before you run the script.
>
> In this case, a temporary print statement for "total" variable explains
> that the loop is iterated 10 times (while processing last revision) and
> the "total" variable is set as 11, so, it removes an item
> unnecessarily.
Sorry, but *NO*, your analysis is wrong.
The loop executes zero times for the inserted item, but *twice* for the
item after the insert point. We ARE changing the childNodes collection
whilst iterating over it. As a result, the iteration is responding in
bizarre ways, because the iterator apparently was not designed to be
capable of responding sensibly to changes in the collection it is
iterating over.
>>> 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.
>
> My point is, we call this function for every revision. So, it's fine to
> remove one item at a time. Moreover, i'm certain the condition is met
> only when processing the last element, so, why we need to check the
> condition for all child nodes.
You need to also consider the case of a user decreasing the --max-items
value in a subsequent invocation of the script..
Max.
Received on Sun Aug 6 14:25:35 2006