Opened 5 years ago

Closed 5 years ago

#4251 defect closed fixed (fixed)

StringTransport's disconnecting instance variable is initialized to zero, not False, contrary to the documentation

Reported by: jesstess Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: jesstess Branch: branches/stringtransport-disconnecting-4251
(diff, github, buildbot, log)
Author: jesstess Launchpad Bug:

Description


Attachments (1)

proto_helpers_false.patch (385 bytes) - added by jesstess 5 years ago.

Download all attachments as: .zip

Change History (10)

Changed 5 years ago by jesstess

comment:1 Changed 5 years ago by jesstess

  • Keywords review added
  • Owner jesstess deleted

Since there's no functional change except to the str() and repr() of disconnecting, can I get away with no unit test and just get a review and apply this on trunk?

comment:2 Changed 5 years ago by exarkun

  • Keywords review removed
  • Owner set to jesstess

It looks like there's current no test coverage for the disconnecting attribute at all. How about adding one that verifies it starts as a false value and becomes true when loseConnection is called.

Presumably the test would pass before the proposed change, so it's not like this is going to prove that False is vastly superior to 0, but as long as someone is looking at this code, we may as well improve test coverage a bit. :)

comment:3 Changed 5 years ago by jesstess

  • Author set to jesstess
  • Branch set to branches/stringtransport-disconnecting-4251

(In [28123]) Branching to 'stringtransport-disconnecting-4251'

comment:4 Changed 5 years ago by jesstess

(In [28124]) Add a docstring, unit test, and topfile.

refs #4251

comment:5 Changed 5 years ago by jesstess

  • Keywords review added
  • Owner jesstess deleted

comment:6 Changed 5 years ago by mwh

  • Keywords review removed

I think using assertIdentical to check that disconnecting really is False, then True would be justified here.

Please merge after making that change.

comment:7 Changed 5 years ago by jesstess

(In [28127]) Use assertFalse and assertTrue.

refs #4251

comment:8 Changed 5 years ago by jesstess

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

(In [28128]) Merge stringtransport-disconnecting-4251

Author: jesstess
Reviewers: mwh
Fixes: #4251

Initialize the disconnecting instance variable of StringTransport to
False instead of 0.

comment:9 Changed 4 years ago by <automation>

Note: See TracTickets for help on using tickets.