Opened 3 years ago

Closed 3 years ago

#5159 enhancement closed fixed (fixed)

gatherResults should have an optional consumeErrors

Reported by: dustin Owned by: exarkun
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/gatherresults-consumeerrors-5159
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

It's been suggested a few times that those who need to use gatherResults, but want any failures in the underlying Deferreds to be handled exclusively by the DeferredList should write their own gatherResults method.

What if this method had an optional consumeErrors=False keyword arugment?

Attachments (2)

bug5159-r1.patch (1.9 KB) - added by dustin 3 years ago.
bug5159-r1.patch
bug5159-r3.patch (6.1 KB) - added by dustin 3 years ago.
bug5159-r3.patch

Download all attachments as: .zip

Change History (13)

Changed 3 years ago by dustin

bug5159-r1.patch

comment:1 Changed 3 years ago by dustin

  • Keywords review added

comment:3 Changed 3 years ago by exarkun

This seems like a duplicate of #5061.

comment:4 Changed 3 years ago by dustin

Not quite (this is a simpler solution to a simpler problem), but I'm sorry I didn't find that bug before filing this one.

comment:5 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to dustin

Okay. Thanks for the quick follow-up (sorry about mine being so delayed).

  1. The gatherResults docstring change for consumeErrors seems a little round-about to me. Do you think it would be better if it just described the behavior of consumeErrors? The current form of trying to proscribe usage seems too narrow to me; it might serve as part of a good example, but not as the canonical documentation for the parameter (I might even say DeferredList's docstring should be canonical, and gatherResults could just refer to that, but I also want to get rid of DeferredList ;).
  2. The test method name should follow the test_something convention, rather than the older testSomething convention.
  3. The test method should have a docstring.
  4. The test method would probably be better if it tested the error handling behavior rather than just checking the flag. Maybe we'll be able to get away with re-implementing gatherResults in terms of the replacement for DeferredList someday, and this attribute won't even exist anymore; the test could be indifferent to that change if it only checks behavior.
  5. It would also be very nice if we had some howto-style documentation that covered gatherResults and this new flag for it. Do you feel like putting together a very quick addition for one of the Deferred howtos to demonstrate this API and the difference between consumeErrors=False and consumeErrors=True? Fortunately doc/core/howto/defer.xhtml does talk about DeferredList and its usage of consumeErrors already, so an extension to that section might not be too hard.

Thanks again! I don't think there should be any problem merging a patch that addresses these issues.

comment:6 Changed 3 years ago by dustin

  • Keywords review added
  • Owner dustin deleted

1: I'm not entirely sure what you mean by "trying to proscribe usage" - does the attached address your concerns?

2-4: done

5: I added a new subsection under DeferredList, treating gatherResults as a shortcut. The docs can change later if gatherResults grows into something that stands alone.

Changed 3 years ago by dustin

bug5159-r3.patch

comment:7 Changed 3 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/gatherresults-consumeerrors-5159

(In [32389]) Branching to 'gatherresults-consumeerrors-5159'

comment:8 Changed 3 years ago by exarkun

(In [32390]) Apply dustin's patch

refs #5159

comment:9 Changed 3 years ago by exarkun

(In [32392]) Address simple coding standard points

  1. Docstring formatting
  2. More description of the behavior being tested in the docstring
  3. camelCase instead of under_scores in variable names
  4. assertIsInstance instead of failUnless(check(...))

refs #5159

comment:10 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to exarkun

Thanks. I made some changes, some linked above, others:

  1. Changed the docs to say >= 11.1 instead of > 11.0
  2. Referred back to DeferredList in the new gatherResults prose docs for the meaning of the consumeErrors parameter.
  3. Made the new test not return a Deferred, since it completes synchronously
  4. Changed the wording in the news fragment to follow the style guide.

The build results look good so far. Assuming the last one completes successfully, I'll merge. Thanks again!

comment:11 Changed 3 years ago by exarkun

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

(In [32405]) Merge gatherresults-consumeerrors-5159

Author: dustin
Reviewer: exarkun
Fixes: #5159

Add a consumeErrors parameter to twisted.internet.defer.gatherResults
with the same meaning as the parameter of the same name accepted by
DeferredList.

Note: See TracTickets for help on using tickets.