String exceptions are being raised in Twisted

[jerub]

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"

[jerub]

branch is kill-raise-string-2063

[exarkun]

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 :)

[jerub]

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"

[exarkun]

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.

[therve]

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

[therve]

(In [21885]) Merge forward

Refs #2063

[therve]

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

Refs #2063

[therve]

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

[exarkun]

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

[therve]

(In [21921]) Add some tests.

Refs #2063

[therve]

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

[exarkun]

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.

[therve]

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

Refs #2063

[therve]

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

Authors: jerub, therve Reviewer: exarkun Fixes #2063

Remove string exceptions raised in the whole twisted codebase.

[therve]

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

Refs #2063

[therve]

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

Refs #2063

[therve]

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

[dreid]

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

[therve]

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

[dreid]

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.

[therve]

(In [22644]) Fix formatting call.

Refs #2063

[dreid]

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.

[therve]

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