Opened 6 years ago

Closed 6 years ago

#3421 defect closed fixed (fixed)

Useless assertion in twisted.web.microdom.Node.replaceChild

Reported by: exarkun Owned by:
Priority: low Milestone: Python-2.6
Component: web Keywords:
Cc: Branch: branches/microdom-assert-3421
(diff, github, buildbot, log)
Author: exarkun, jparise Launchpad Bug:

Description

The assertion is always positive. Plus asserts are almost always pointless.

Attachments (2)

microdom.assert.diff (668 bytes) - added by jparise 6 years ago.
Corrects assert syntax
microdom.assert2.diff (4.4 KB) - added by jparise 6 years ago.
Replaces asserts with exceptions

Download all attachments as: .zip

Change History (18)

Changed 6 years ago by jparise

Corrects assert syntax

comment:1 Changed 6 years ago by jparise

  • Keywords review added

The attached patch corrects the syntax of the offending assert statement. This assert now functions as intended and no longer generates warnings under Python 2.6+. There should be no other behavioral differences.

comment:2 Changed 6 years ago by exarkun

  • Keywords review removed
  • Owner changed from jknight to jparise

Asserts are silly. Can you instead change it to an explicit check which raises some documented interface and add a unit test for the behavior?

Changed 6 years ago by jparise

Replaces asserts with exceptions

comment:3 Changed 6 years ago by jparise

  • Keywords review added

I've attached a new patch that replaces a handful of assertions with exceptions. The unit test has been updated, and the exceptions are documented in the methods' docstrings.

There are still some remaining assertions in microdom.py that could also be replaced with exceptions. Those cases are more "interesting", however, as they would benefit most from custom exception classes given their microdom-ish-specific nature. If there's a desire to convert those as well, it might be best to track that work under a separate ticket.

comment:4 Changed 6 years ago by jparise

  • Milestone set to Python-2.6

Adding the Python-2.6 milestone because the current trunk code produces a warning under Python 2.6.

comment:5 Changed 6 years ago by exarkun

  • Owner jparise deleted

comment:6 Changed 6 years ago by exarkun

  • Author set to jparise

comment:7 Changed 6 years ago by exarkun

  • Author changed from jparise to exarkun, jparise
  • Branch set to branches/microdom-assert-3421

(In [25490]) Branching to 'microdom-assert-3421'

comment:8 Changed 6 years ago by exarkun

(In [25491]) apply microdom.assert2.diff

refs #3421

comment:9 Changed 6 years ago by exarkun

(In [25493]) split replaceChild ValueError test into separate method

refs #3421

comment:10 Changed 6 years ago by exarkun

(In [25494]) remove useless dom parent fakes

refs #3421

comment:11 Changed 6 years ago by exarkun

(In [25495]) raise ValueError from Document.appendChild if there are too many children

refs #3421

comment:12 Changed 6 years ago by exarkun

(In [25496]) some docstrings

refs #3421

comment:13 Changed 6 years ago by exarkun

thanks jparise.

I've made a few more changes in the branch. Someone else should take a look at this now.

comment:14 Changed 6 years ago by mwh

  • Keywords review removed

The exception in the branch make more sense than the assertions in trunk. They're still not very good, as they don't really provide much help in fixing your problem (like, what type was it if it wasn't a Node? For replaceChild, which parameter was it?).

OTOH, microdom is on it's way out anyway (right?), so I don't see much point in expending effort in fixing the above.

Land the branch, IOW.

comment:15 Changed 6 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(In [25554]) Merge microdom-assert-3421

Author: jparise, exarkun
Reviewer: exarkun, mwhudson
Fixes: #3421

Remove most asserts from microdom and replace them with checks which raise some
documented exception. In particular, remove one assertion which could never
fail and which produced a warning on Python 2.6.

comment:16 Changed 3 years ago by <automation>

Note: See TracTickets for help on using tickets.