Merge lp://staging/~jkakar/storm/better-timeout-messages into lp://staging/storm

Proposed by Jamu Kakar
Status: Merged
Approved by: Jamu Kakar
Approved revision: 371
Merged at revision: 370
Proposed branch: lp://staging/~jkakar/storm/better-timeout-messages
Merge into: lp://staging/storm
Diff against target: 135 lines (+42/-10)
5 files modified
storm/databases/postgres.py (+2/-1)
storm/exceptions.py (+4/-2)
storm/tracer.py (+20/-2)
tests/databases/postgres.py (+3/-2)
tests/tracer.py (+13/-3)
To merge this branch: bzr merge lp://staging/~jkakar/storm/better-timeout-messages
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Approve
Robert Collins (community) Approve
Storm Developers Pending
Review via email: mp+32687@code.staging.launchpad.net

Description of the change

This branch introduces the following changes:

- Adds a description when TimeoutError's are raised.

- Improves TimeoutTracer docstrings.

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote :

Looks great to me. I don't know the code well enough to spot any flaws :)

The TimeoutTracer docstring could be a little clearer in the first sentence.

Perhaps

"""Provide a timeout facility for connections to prevent rogue operations.

...
"""

review: Approve
371. By Jamu Kakar

- Improve docstring.

Revision history for this message
Jamu Kakar (jkakar) wrote :

Thanks Rob, I've updated the docstring to what you've suggested.

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

Hi, this looks fine and a clear improvement.

Is there any chance that changing the signature of TimeoutError.__init__ will break client code? It does seem unlikely, but I thought it was worth asking :-)

review: Approve
Revision history for this message
Jamu Kakar (jkakar) wrote :

Michael:

I wondered about the changes to TimeoutError.__init__ breaking
client code, too. The sense I have is that it would be fairly odd
for client code to use what is basically an internal Storm exception
type. I suspect that any non-Storm code using it is doing something
funky that we don't need to worry about supporting.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to status/vote changes: