Opened 4 years ago

Last modified 17 months ago

#4907 defect assigned

DeferredSemaphore __repr__ enhancement

Reported by: lewq Owned by: zintinio
Priority: normal Milestone:
Component: core Keywords: easy
Cc: thijs Branch:
Author: Launchpad Bug:

Description

It's easier to get a handle on DeferredSemaphore if you can print one and get meaningful statistics out of it:

    def __str__(self):
        return "<DeferredSemaphore with %i jobs max, %i jobs waiting, %i jobs running>" % (
                self.limit, len(self.waiting), self.limit - self.tokens)

Adding the above enhancement, and adding this code to the documentation, will make it easier for a beginner to see what's going on:

sem = DeferredSemaphore(tokens=10)

def job(num):
    # Simulate a long-running task
    d = defer.Deferred()
    reactor.callLater(1, d.callback, num)
    return d

# Add 100 jobs to the queue
for i in range(100):
    print sem
    d = sem.run(job, i)
    def job_done(data):
        print "Job %i completed" % data
        print sem
    d.addCallback(job_done)

reactor.run()

Attachments (1)

4907.patch (2.1 KB) - added by zintinio 17 months ago.
Initial patch adding repr to DeferredSemaphore

Download all attachments as: .zip

Change History (11)

comment:1 Changed 4 years ago by thijs

  • Cc thijs added

Also see #1432.

comment:2 Changed 4 years ago by exarkun

  • Owner set to lewq

Yea, this would be nice. I'd suggest something a little less verbose though, typical convention suggests something like:

<DeferredSemaphore max=%d running=%d waiting=%d>

Also a unit test, of course. ;)

comment:3 Changed 17 months ago by Julian

  • Keywords easy added; deferredsemaphore removed
  • Summary changed from DeferredSemaphore __str__ enhancement to DeferredSemaphore __repr__ enhancement

This should be for __repr__, not __str__.

comment:4 Changed 17 months ago by Julian

  • Owner lewq deleted

comment:5 Changed 17 months ago by zintinio

  • Owner set to zintinio
  • Status changed from new to assigned

Changed 17 months ago by zintinio

Initial patch adding repr to DeferredSemaphore

comment:6 Changed 17 months ago by zintinio

  • Keywords review added
  • Owner zintinio deleted
  • Status changed from assigned to new

comment:7 Changed 17 months ago by exarkun

The implementation might be simpler using twisted.python.util.FancyStrMixin.

comment:8 Changed 17 months ago by zintinio

Using twisted.python.util.FancyStrMixin, it's actually more complicated to display the information. To use FancyStrMixin, the child class must override showAttributes, and list the display attributes. This is fine for simple cases, but in order to calculate running, you have to subtract self.limit - self.tokens, but passing a callable to FancyStrMixin won't work because it has no access to the instance variables.

For example:

showAttributes = (('limit', 'max', '%s'),                          
                  ('waiting', lambda w: str(len(w))),
                  ('running', ...),  # no attribute running
                  ('limit', lambda l: limit - self.tokens)) # no access to self, displays "limit"
                  ('self', ...)      # no access to self
                  

The patch I submitted is better than using a mixin in this case.

comment:9 follow-up: Changed 17 months ago by exarkun

  • Keywords review removed
  • Owner set to zintinio

Thanks for your work on this.

A private property is an easy way to make the computed values available. For example:

@property
def _running(self):

return self.limit - self.tokens

This also gives you a simpler interface to write unit tests for. The current patch only has one unit test, so there's no demonstration that the computed values are actually computed correctly. And on that topic, there's actually no demonstration that the other values are correctly included either: the current unit test will pass even if the implementation returns a hard-coded string.

The unit test also shouldn't generate the correct string dynamically. Just hard-code the correct result. This makes the test simpler and less likely to have bugs.

I'd also skip renaming the values (eg, renaming "limit" to "max" - these words are synonyms, there's no reason to not use the name of the __init__ parameter in the string representation, it just introduces the possibility for confusion).

I think FancyStrMixin will still lead to less complexity, but I don't think using it is mandatory. Please do add the extra unit tests I mentioned, though. Thanks again!

comment:10 in reply to: ↑ 9 Changed 17 months ago by zintinio

  • Status changed from new to assigned

Replying to exarkun:

Thanks for your work on this.

A private property is an easy way to make the computed values available. For example:

@property
def _running(self):

return self.limit - self.tokens

This also gives you a simpler interface to write unit tests for. The current patch only has one unit test, so there's no demonstration that the computed values are actually computed correctly. And on that topic, there's actually no demonstration that the other values are correctly included either: the current unit test will pass even if the implementation returns a hard-coded string.

The unit test also shouldn't generate the correct string dynamically. Just hard-code the correct result. This makes the test simpler and less likely to have bugs.

I'd also skip renaming the values (eg, renaming "limit" to "max" - these words are synonyms, there's no reason to not use the name of the __init__ parameter in the string representation, it just introduces the possibility for confusion).

I think FancyStrMixin will still lead to less complexity, but I don't think using it is mandatory. Please do add the extra unit tests I mentioned, though. Thanks again!

Thank you for your feedback. I will send my revised within the next 24 hours.

Note: See TracTickets for help on using tickets.