Ticket #4538 enhancement closed fixed

Opened 4 years ago

Last modified 4 years ago

Use False/True instead of 0/1 in twisted/internet/defer.py

Reported by: exarkun Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/booleans-in-defer-4538
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

Booleans are great, I suppose. Clarity of intent, or something like that. Deferred.called, Deferred._suppressAlreadyCalled, DeferredList.fireOnOne{Callback,Errback}, DeferredLock.locked`... these aren't integers on which to perform arithmetic. They're flags indicating one of two possible states.

(There seems to be some desire to do this in the #411 branch; so to shorten the diff there, I'm moving those changes here.)

Change History

1

  Changed 4 years ago by exarkun

  • branch set to branches/booleans-in-defer-4538
  • branch_author set to exarkun

(In [29561]) Branching to 'booleans-in-defer-4538'

2

follow-up: ↓ 3   Changed 4 years ago by exarkun

  • owner exarkun deleted
  • keywords review added

Fairly straightforward.  Documentation builder result

3

in reply to: ↑ 2   Changed 4 years ago by cyli

  • keywords review removed
  • owner set to exarkun

In defer.gatherResults, DeferredList is still initialized with fireOnOneErrback = 1 instead of with a boolean. Since this is in the defer module itself, it should probably be changed to be fireOnOneErrback = True.

However, should the rest of the code in Twisted be modified to use booleans when using deferreds? Current usage is inconsistent - sometimes booleans are used, sometimes 0/1 flags. For instance, twisted.conch.scripts.cftp uses the flags, but twisted.conch.test.test_transport uses booleans. 0/1 flags will still work (all tests pass, and  build results are fine). Perhaps if that is not in the scope of this branch a new ticket should be opened to change usage of flags in deferreds to booleans.

Other than these two points, looks good to merge.

4

  Changed 4 years ago by exarkun

(In [29580]) Change gatherResults to use False and True instead of 0 and 1

refs #4538

5

  Changed 4 years ago by exarkun

  • status changed from new to assigned

In defer.gatherResults, DeferredList is still initialized with fireOnOneErrback = 1

Fixed in r29580, along with an =0 in nearby code.

However, should the rest of the code in Twisted be modified to use booleans when using deferreds?

Not now. This change makes sense because of #411 (or at least I'm assuming it does, since another developer made these changes in the branch for that ticket). I've split them out to make the job of reviewing #411 easier. This shouldn't be taken as a sign that we should go off to the rest of Twisted and start changing 1s to Trues and so on.

Where such a change will make maintenance easier, great. But there's no point in doing it just to do it.

Also, there's no compatibility issue with code in Twisted continuing to pass 1 into these APIs rather than True. The values are almost completely interchangable, and their differences (such as the result of using str() or repr() on them) won't make any difference to this code.

Thanks for the review!

6

  Changed 4 years ago by exarkun

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

(In [29581]) Merge booleans-in-defer-4538

Author: therve, exarkun Reviewer: cyli Fixes: #4538

Document some of the attributes of Deferred and switch over most (perhaps all) of defer.py to using actual booleans where it makes sense to do so.

7

  Changed 3 years ago by <automation>

  • owner exarkun deleted
Note: See TracTickets for help on using tickets.