DeferredSemaphore __repr__ enhancement

[lewq]

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

[thijs]

Also see #1432.

[exarkun]

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. ;)

[Julian]

This should be for __repr__, not __str__.

[zintinio]

Initial patch adding repr to DeferredSemaphore

[exarkun]

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

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

[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!

[zintinio]

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.