Ticket #3421 defect closed fixed

Opened 5 years ago

Last modified 4 years ago

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
Author: exarkun, jparise Launchpad Bug:

Description

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

Attachments

microdom.assert.diff Download (0.7 KB) - added by jparise 5 years ago.
Corrects assert syntax
microdom.assert2.diff Download (4.4 KB) - added by jparise 4 years ago.
Replaces asserts with exceptions

Change History

Changed 5 years ago by jparise

Corrects assert syntax

1

Changed 5 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.

2

Changed 5 years ago by exarkun

  • owner changed from jknight to jparise
  • keywords review removed

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 4 years ago by jparise

Replaces asserts with exceptions

3

Changed 4 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.

4

Changed 4 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.

5

Changed 4 years ago by exarkun

  • owner jparise deleted

6

Changed 4 years ago by exarkun

  • branch_author set to jparise

7

Changed 4 years ago by exarkun

  • branch set to branches/microdom-assert-3421
  • branch_author changed from jparise to exarkun, jparise

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

8

Changed 4 years ago by exarkun

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

refs #3421

9

Changed 4 years ago by exarkun

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

refs #3421

10

Changed 4 years ago by exarkun

(In [25494]) remove useless dom parent fakes

refs #3421

11

Changed 4 years ago by exarkun

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

refs #3421

12

Changed 4 years ago by exarkun

(In [25496]) some docstrings

refs #3421

13

Changed 4 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.

14

Changed 4 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.

15

Changed 4 years ago by exarkun

  • status changed from new to closed
  • resolution set to fixed

(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.

16

Changed 2 years ago by <automation>

Note: See TracTickets for help on using tickets.