Opened 4 years ago

Closed 4 years ago

#4538 enhancement closed fixed (fixed)

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

comment:1 Changed 4 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/booleans-in-defer-4538

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

comment:2 follow-up: Changed 4 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

Fairly straightforward. Documentation builder result

comment: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.

comment:4 Changed 4 years ago by exarkun

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

refs #4538

comment: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!

comment:6 Changed 4 years ago by exarkun

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

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

comment:7 Changed 3 years ago by <automation>

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