Ticket #2063 (closed defect: fixed )

Opened 3 years ago

Last modified 1 year ago

String exceptions are being raised in Twisted

Reported by: jerub Assigned to: therve
Type: defect Priority: highest
Milestone: Component: core
Keywords: Cc: therve
Branch: branches/kill-raise-string-2063-3 Author: therve
Launchpad Bug:

Description

Every single one of these lines found with this grep should be eliminated:

shiny% grep raise\ \[\"\'] **/*.py
doc/core/examples/row_util.py:            raise "Key factory key pool exceeded."
twisted/conch/scripts/tkconch.py:        raise "can't ask 2 questions at once!"
twisted/conch/ssh/asn1.py:            raise 'unknown type %s' % type(part)
twisted/persisted/aot.py:                    raise "instance method changed"
twisted/persisted/aot.py:                raise "Unsupported type: %s" % objType.__name__
twisted/persisted/crefutil.py:            raise 'waht!?', n, obj
twisted/persisted/marmalade.py:                raise "instance method changed"
twisted/persisted/marmalade.py:                            raise "Unjellying Error: key role not set"
twisted/persisted/marmalade.py:            raise "Unsupported Node Type: %s" % str(node.tagName)
twisted/persisted/marmalade.py:                raise "Unsupported type: %s" % objType.__name__
twisted/spread/jelly.py:            raise 'instance method changed'
twisted/test/test_failure.py:            raise "bugger off"
twisted/test/test_pb.py:        raise "factory copy state has no 'id' member %s" % repr(state)
twisted/test/test_pb.py:        raise "factory class has no ID: %s" % SimpleFactoryCopy.allIDs
twisted/test/test_pb.py:        raise "factory method found no object with id"
twisted/web2/test/test_http.py:            raise "Your computer's clock is way wrong, this test will be invalid."
twisted/words/xish/xpath.py:        raise "UnsupportedOperation"

Attachments

Change History

  2006-09-06 12:31:39+00:00 changed by jerub

  • keywords set to review
  • priority changed from normal to highest

branch is kill-raise-string-2063

  2006-09-07 01:19:00+00:00 changed by jerub

  • owner deleted

  2006-09-07 02:23:44+00:00 changed by exarkun

  • keywords deleted
  • owner set to jerub

This looks like the start of a really nice cleanup. Since no tests changed (except the ones that were raising string exceptions themselves) and they're all still passing, I suppose none of these exceptional codepaths have any test coverage.

I very much hope that no one is invoking these codepaths, but they should probably have some test coverage anyhow.

Also the AssertionErrors? raised outside of the test suite should probably be runtime errors or something like that. Twisted doesn't really have a policy on where asserts can/should be used, so I always feel better not using them. That way no one can ever argue that they're being used inappropriately :)

  2006-09-08 01:56:36+00:00 changed by jerub

  • keywords set to review
  • owner changed from jerub to exarkun

I've changed some of the AssertionErrors? to RuntimeErrors?. I changed one to NotImplementedError? too (it had a test, i changed the test to match).

I also found this:

assert myExpression(), AssertionError("foo")

which is quite odd. I changed it to:

assert myExpression(), "foo"

  2006-09-09 13:31:02+00:00 changed by exarkun

  • keywords deleted
  • owner changed from exarkun to jerub

Functions with raise changes without test coverage:

  • twisted/conch/scripts/tkconch.py's deferredAskFrame
  • The else/raise ValueError? in twisted/conch/ssh/asn1.py's pack
  • twisted/words/xish/xpath.py's _AnyLocation.queryForString
  • twisted/web2/dav/fileop.py's copy (oh man, the entire function is uncovered, and it is a huuuuuge function)
  • twisted/persisted/crefutil.py's _DictKeyAndValue._setitem
  • twisted/persisted/crefutil.py's _Defer.setitem
  • twisted/persisted/aot.py's AOTUnjellier.unjellyAO
  • twisted/persisted/aot.py's AOTJellier.jellyToAO
  • twisted/persisted/marmalade.py's DOMUnjellier.unjellyNode (two changes, both uncovered)
  • twisted/persisted/marmalade.py's DOMJellier.jellyToNode
  • twisted/flow/stage.py's Map._yield
  • twisted/spread/jelly.py's _Unjellier._unjelly_method

Many of these are in code which is deprecated or which will soon be deprecated. If you want to create a separate ticket for improving the test coverage of those and not add tests in this branch, that's fine. Conch, Words, Web2, and core (twisted/spread/) should all have tests added for these code changes, though.

twisted.mail.mail.BounceDomain?.startMessage's docstring uses triple single quotes, please change to triple double quotes.

All the other actual changes you made look good.

  2007-11-20 11:01:37+00:00 changed by therve

  • branch set to branches/kill-raise-string-2063-2
  • author set to therve

(In [21884]) Branching to 'kill-raise-string-2063-2'

  2007-11-20 11:02:12+00:00 changed by therve

  • owner changed from jerub to therve

  2007-11-20 11:03:13+00:00 changed by therve

(In [21885]) Merge forward

Refs #2063

  2007-11-20 11:42:50+00:00 changed by therve

(In [21886]) Add tests for the parts of code still usable.

Refs #2063

  2007-11-20 11:44:42+00:00 changed by therve

  • cc set to therve
  • keywords set to review
  • owner deleted
  • author changed from therve to therve, jerub

I've made some of the cleanups. I hope it will be useful!

  2007-11-20 19:18:51+00:00 changed by exarkun

  • keywords deleted
  • owner set to therve

Thanks!

  • TkConchTestCase might benefit from TestCase.patch
  • test_methodsNotSelfIdentity clobbers C.cmethod
  • Some things still need tests:
    • twisted.persisted.aot (existing tests are in test_persisted)
    • twisted.persisted.crefutil
    • twisted.persisted.marmalade (existing tests are in test_persisted)
    • twisted.persisted.dirdbm
    • twisted.flow.stage (question whether this change is even worth making - how much longer is flow going to live?)
  • Maybe testIfModifiedSince should use self.fail - although it's kind of dumb that it can't deal with this case.
  • queryForString should raise a NotImplementedError with a message

  2007-11-20 22:18:49+00:00 changed by therve

(In [21921]) Add some tests.

Refs #2063

  2007-11-20 22:20:43+00:00 changed by therve

  • keywords set to review
  • owner deleted

I've done everything, except the one in flow, and the last one in marmalade.

  2007-12-27 23:08:13+00:00 changed by exarkun

  • owner set to exarkun
  • status changed from new to assigned

  2007-12-27 23:16:32+00:00 changed by exarkun

  • keywords deleted
  • owner changed from exarkun to therve
  • status changed from assigned to new

I suggest completely reverting the twisted.flow change, given the absence of tests and the small likelihood that there will even be another twisted.flow release.

dirdbm should probably be raising TypeError, not AssertionError. I'll be happy enough if you just add a note to this effect to the new unit test which now verifies it raises AssertionError. Changing the behavior probably isn't really worthwhile, but avoiding creating a new example which someone might mistake as best practice is.

Otherwise, everything looks good.

  2007-12-28 12:43:56+00:00 changed by therve

(In [22164]) Revert change in flow, add a note about the awkward use of AssertionError? in dirdbm.

Refs #2063

  2007-12-28 12:56:15+00:00 changed by therve

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

(In [22165]) Merge kill-raise-string-2063-2

Authors: jerub, therve Reviewer: exarkun Fixes #2063

Remove string exceptions raised in the whole twisted codebase.

  2007-12-28 13:09:18+00:00 changed by therve

(In [22166]) Revert r22165: bad dependancy on tkinter.

Refs #2063

  2007-12-28 13:13:09+00:00 changed by therve

(In [22167]) Add a skip when tk is not present.

Refs #2063

  2007-12-28 13:13:39+00:00 changed by therve

  • status changed from closed to reopened
  • resolution deleted

  2007-12-28 13:14:25+00:00 changed by therve

  • keywords set to review
  • owner deleted
  • status changed from reopened to new

OK, that was a bit fast, that probably deserves a last round of review.

  2008-02-20 21:19:10+00:00 changed by dreid

  • keywords deleted
  • owner set to therve

Merge conflicts in C: C twisted/test/test_jelly.py C: C twisted/spread/jelly.py

  2008-02-20 21:28:40+00:00 changed by therve

  • branch changed from branches/kill-raise-string-2063-2 to branches/kill-raise-string-2063-3
  • author changed from therve, jerub to therve

(In [22641]) Branching to 'kill-raise-string-2063-3'

  2008-02-20 21:33:25+00:00 changed by therve

  • keywords set to review
  • owner deleted

  2008-02-20 21:37:02+00:00 changed by dreid

  • keywords deleted
  • owner set to therve

The check in test_http.py bothers me a great deal but fixing that is outside of the scope of this ticket.

There are also a number of lines touched here where the % operator is used without a tuple on the right side.

asn1.py

            raise ValueError('unknown type %s' % type(part))

marmalade.py

            raise TypeError("Unsupported Node Type: %s" % str(node.tagName))
                raise TypeError("Unsupported type: %s" % objType.__name__)

I think it's best to clean up these so as not to give the wrong impression that this is acceptable behavior.

  2008-02-20 21:43:33+00:00 changed by therve

(In [22644]) Fix formatting call.

Refs #2063

  2008-02-21 16:39:53+00:00 changed by therve

  • keywords set to review
  • owner deleted

  2008-02-21 22:39:47+00:00 changed by dreid

  • keywords deleted
  • owner set to therve

You've satisfied my issues, I vote you build the branch on all the supported slaves and then merge.

As always, thanks for all the work therve.

  2008-02-22 10:13:16+00:00 changed by therve

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

(In [22687]) Merge kill-raise-string-2063-4

Authors: jerub, therve Reviewers: dreid, exarkun Fixes #2063

Remove string exceptions raised in the whole twisted codebase.

The first merge added an useless dependency on tk in tests, corrected since.

Note: See TracTickets for help on using tickets.