[issue2170] rewrite of minidom.Node.normalize

[R. David Murray]

R. David Murray  added the comment:

Committed in r71414.

--
resolution:  -> accepted
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue2170] rewrite of minidom.Node.normalize

[R. David Murray]

Changes by R. David Murray :


--
versions:  -Python 2.6, Python 3.0

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue2170] rewrite of minidom.Node.normalize

[A.M. Kuchling]

A.M. Kuchling  added the comment:

The new version of the code looks all right, so I
think this patch can be applied.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue2170] rewrite of minidom.Node.normalize

[R. David Murray]

R. David Murray  added the comment:

Andrew, any objection to my going ahead and applying this patch?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue2170] rewrite of minidom.Node.normalize

[R. David Murray]

Changes by R. David Murray :


--
stage: patch review -> commit review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue2170] rewrite of minidom.Node.normalize

[Brett Cannon]

Changes by Brett Cannon :


--
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue2170] rewrite of minidom.Node.normalize

[Malte Helmert]

Malte Helmert  added the comment:

Short review: code looks good to me, patch applies cleanly to trunk,
passes tests.

@akuchling: I don't know if you remember, but this rewrite was
originally suggested by you on a bug day some time ago. I think David's
patch is ready to be applied.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue2170] rewrite of minidom.Node.normalize

[R. David Murray]

R. David Murray  added the comment:

Removed old patches, updated patch to remvoe the unnecessary local
variable assignment (also pushed to launchpad).

--
Added file: http://bugs.python.org/file13374/issue2170.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue2170] rewrite of minidom.Node.normalize

[R. David Murray]

Changes by R. David Murray :


Removed file: http://bugs.python.org/file13352/issue2170.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue2170] rewrite of minidom.Node.normalize

[R. David Murray]

Changes by R. David Murray :


Removed file: http://bugs.python.org/file13312/test_minidom.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue2170] rewrite of minidom.Node.normalize

[Malte Helmert]

Malte Helmert  added the comment:

I removed my original patch which has been superseded by David's patch.

David, I suggest that you also remove test_minidom.patch which is
superseded by your later patch (see http://bugs.python.org/msg77766 for
policy on this).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue2170] rewrite of minidom.Node.normalize

[Malte Helmert]

Changes by Malte Helmert :


Removed file: http://bugs.python.org/file9513/minidom.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue2170] rewrite of minidom.Node.normalize

[Malte Helmert]

Malte Helmert  added the comment:

While we're cleaning up:
   data = child.data
   if not data:
could be
   if not child.data:
since data is not used again.

Alternatively, you could use data in place of child.data later on for a
small speed-up, but I doubt that we care about such small optimizations
here. If we do, then we should also assign a name to child.nextSibling,
though.

I found the three-line duplication between the two branches mildly
refactor-worthy, but when I eliminated it the control logic got more
involved and I don't think that's a good trade-off here.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue2170] rewrite of minidom.Node.normalize

[R. David Murray]

Changes by R. David Murray :


Removed file: http://bugs.python.org/file13349/issue2170.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue2170] rewrite of minidom.Node.normalize

[R. David Murray]

R. David Murray  added the comment:

Added several more tests, to validate the sibling link code more, and
fixed the bug.

--
Added file: http://bugs.python.org/file13352/issue2170.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue2170] rewrite of minidom.Node.normalize

[R. David Murray]

R. David Murray  added the comment:

You are absolutely right.  I will write a test case and fix the patch.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue2170] rewrite of minidom.Node.normalize

[Malte Helmert]

Malte Helmert  added the comment:

I eyeballed the new patch and wonder if the case where text nodes are
collapsed is handled correctly. Assume that self.childNodes contains
nodes [A, B, C], where A and B are non-empty text nodes and C is a
non-text node.

The algorithm would collapse A and B into A and then unlink B, and I
think C.previousSibling would still reference the unlinked B, rather
than the correct A. Am I missing something?

If this is indeed a bug in the proposed patch, I suggest adding an
additional test for this case.

The bug itself should not be too hard to fix; the "elif" case would need
the same

if child.nextSibling:
child.nextSibling.previousSibling = child.previousSibling

assignment as the "if" case.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue2170] rewrite of minidom.Node.normalize

[R. David Murray]

R. David Murray  added the comment:

Er, I meant to say, "as easy to understand" as the proposed patch.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue2170] rewrite of minidom.Node.normalize

[R. David Murray]

R. David Murray  added the comment:

I checked the speed of the proposed patch, and found that it was
definitely slower than the original code.  So I took another look at the
original, and refactored it in a different way: instead of moving the
sibling relinking into a second pass, I changed to code to only relink
siblings when a node is removed.  The new patch passes all test, and is
faster than the old code.  I tested the timing both against the same
small nested document I used in testNormalize2, and by running normalize
on a 37K html document (a copy of the xml.dom.minidom chapter from the
Library Reference):

original code:
testNormalize2: [2.5144219398498535, 2.5053589344024658, 2.5059471130371094]
example.html:   [44.641155958175659, 44.575434923171997, 44.996657133102417]

original patch
testNormalize2: [2.7070891857147217, 2.7012341022491455, 2.7003159523010254]
example.html:   [67.908604860305786, 68.088788986206055, 67.92288613319397]

My patch
testNormalize2: [2.4626028537750244, 2.4619381427764893, 2.4617609977722168]
example.html:   [22.780415058135986, 22.780103921890259, 22.721666097640991]

IMO my refactoring is also easier to understand than either the old code
or the proposed patch.

Patch, including new test, is attached, and also pushed to
bzr+ssh://bazaar.launchpad.net/~rdmurray/python/issue2170.

--
versions: +Python 2.7, Python 3.0, Python 3.1
Added file: http://bugs.python.org/file13349/issue2170.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue2170] rewrite of minidom.Node.normalize

[R. David Murray]

R. David Murray  added the comment:

I have applied the supplied patch to the current trunk, and it passes
the tests.  I've also attached a more extensive test case that exercises
the recursion.

--
keywords: +patch
nosy: +bitdancer
Added file: http://bugs.python.org/file13312/test_minidom.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue2170] rewrite of minidom.Node.normalize

[A.M. Kuchling]

Changes by A.M. Kuchling:


--
assignee:  -> akuchling
nosy: +akuchling

__
Tracker <[EMAIL PROTECTED]>

__
___
Python-bugs-list mailing list 
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue2170] rewrite of minidom.Node.normalize

[Malte Helmert]

New submission from Malte Helmert:

In the discussion of #1433694 on the #python-dev channel, it was
observed that the normalize method of minidom.Node could take some
refactoring. A patch is attached.

--
components: XML
files: minidom.diff
messages: 62794
nosy: maltehelmert
severity: normal
status: open
title: rewrite of minidom.Node.normalize
type: feature request
versions: Python 2.6
Added file: http://bugs.python.org/file9513/minidom.diff

__
Tracker <[EMAIL PROTECTED]>

__
___
Python-bugs-list mailing list 
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com