Opened 6 years ago

Last modified 4 years ago

#4907 defect assigned

DeferredSemaphore __repr__ enhancement

Reported by: lewq Owned by: Arthur Maciejewicz
Priority: normal Milestone:
Component: core Keywords: easy
Cc: Thijs Triemstra Branch:
Author:

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 Arthur Maciejewicz 4 years ago.
Initial patch adding repr to DeferredSemaphore

Download all attachments as: .zip

Change History (11)

comment:1 Changed 6 years ago by Thijs Triemstra

Cc: Thijs Triemstra added

Also see #1432.

comment:2 Changed 6 years ago by Jean-Paul Calderone

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 4 years ago by Julian Berman

Keywords: easy added; deferredsemaphore removed
Summary: DeferredSemaphore __str__ enhancementDeferredSemaphore __repr__ enhancement

This should be for __repr__, not __str__.

comment:4 Changed 4 years ago by Julian Berman

Owner: lewq deleted

comment:5 Changed 4 years ago by Arthur Maciejewicz

Owner: set to Arthur Maciejewicz
Status: newassigned

Changed 4 years ago by Arthur Maciejewicz

Attachment: 4907.patch added

Initial patch adding repr to DeferredSemaphore

comment:6 Changed 4 years ago by Arthur Maciejewicz

Keywords: review added
Owner: Arthur Maciejewicz deleted
Status: assignednew

comment:7 Changed 4 years ago by Jean-Paul Calderone

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

comment:8 Changed 4 years ago by Arthur Maciejewicz

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 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Arthur Maciejewicz

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 4 years ago by Arthur Maciejewicz

Status: newassigned

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.