Opened 7 years ago

Last modified 4 years ago

#4623 defect new

Missing interface IQueue

Reported by: Pepijn de Vos Owned by: Stacey Sern
Priority: low Milestone:
Component: mail Keywords:
Cc: moijes12 Branch: branches/add-IRelayQueue-interface-4623
branch-diff, diff-cov, branch-cov, buildbot
Author: tomprince

Description

in twisted.mail.relaymanager there is a class named Queue, which should implement IQueue according to SmartHostSMTPRelayingManager.

Queue: http://twistedmatrix.com/trac/browser/tags/releases/twisted-10.1.0/twisted/mail/relaymanager.py#L134

Class stating to accept an implementer of IQueue: http://twistedmatrix.com/trac/browser/tags/releases/twisted-10.1.0/twisted/mail/relaymanager.py#L342

Attachments (4)

iqueue.patch (1.7 KB) - added by Pepijn de Vos 7 years ago.
First attempt at a patch. The whole Queue thing seems like a piece of crap to me.
4623.patch (4.3 KB) - added by moijes12 5 years ago.
Used pepijn's patch, Glyph's comments and some time to create this patch. I was not so sure how to create the unit test and how to name it. Suggestions for improvements would be welcome.
4623.2.patch (4.9 KB) - added by moijes12 5 years ago.
4623.3.patch (5.6 KB) - added by moijes12 5 years ago.

Download all attachments as: .zip

Change History (22)

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

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Pepijn de Vos

I don't see anything to review here.

comment:2 in reply to:  1 Changed 7 years ago by Pepijn de Vos

Replying to exarkun:

I don't see anything to review here.

I'm sorry. I'm not very experience with reporting bugs and patches and stuff, I was told to attach the review tag to a bug last time, so I thought I was supposed to do that again.

Also, what does owner mean? Does that mean I'm supposed to write the IQueue interface and submit a patch? If so, I'll try to of course, I heard rumours about fighting bears :)

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

I'm sorry. I'm not very experience with reporting bugs and patches and stuff, I was told to attach the review tag to a bug last time, so I thought I was supposed to do that again.

See ReviewProcess

Also, what does owner mean? Does that mean I'm supposed to write the IQueue interface and submit a patch?

Yes, pretty much. :)

Changed 7 years ago by Pepijn de Vos

Attachment: iqueue.patch added

First attempt at a patch. The whole Queue thing seems like a piece of crap to me.

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

Keywords: review added

Now you should add the review keyword.

comment:5 Changed 7 years ago by Glyph

Keywords: review removed

pepjin, thanks for your contribution :). Here's your review! In order to get this accepted, you need to respond to the feedback here, post a new patch, add the 'review' keyword again, and then re-assign it to nobody (the empty entry at the top of the 'assign to' list).

  1. The twisted convention for docstrings is that they be formatted like this:
    """
    Hello, world.
    """
    
    not like this:
    """Hello, world."""
    
  2. "XXX" comments in the code are strongly discouraged. Instead, file a ticket outlining your specific concern (what's bad about it being file oriented, what could change to make it less file oriented) and then include a link to that ticket. Ideally you should use the epytext syntax for linking rather than sticking it in a comment, if this might be of interest to users of the public API. See here for an explanation of that syntax.
  3. There should always be tests for new code. This IQueue interface isn't doing a lot, so there don't have to be a lot of tests, but you can use zope.interface.verify.veryObject and verifyClass to make sure that the interface actually matches the implementation.
  4. The use of an interface is largely in documentation, but this interface doesn't say what most of its required parameters are or what types its method returns. Look for other Twisted stuff that uses the @param, @type, @return, and @rtype declarations in their docstrings.

Thanks again!

comment:6 Changed 5 years ago by moijes12

Cc: moijes12 added

Changed 5 years ago by moijes12

Attachment: 4623.patch added

Used pepijn's patch, Glyph's comments and some time to create this patch. I was not so sure how to create the unit test and how to name it. Suggestions for improvements would be welcome.

comment:7 Changed 5 years ago by moijes12

Keywords: review added
Owner: Pepijn de Vos deleted

comment:8 Changed 5 years ago by David Reid

Owner: set to David Reid
Status: newassigned

comment:9 Changed 5 years ago by David Reid

Keywords: review removed
Owner: changed from David Reid to moijes12
Status: assignednew

Thanks for working on this moijes12!

  • C{object} is too general, if the return value is a file object C{file} is probably appropriate.
  • @return: None should be @return: C{None}
  • @rtype: C{None} should be @rtype: C{NoneType}
  • Your patch introduces trailing whitespace and whitespace on blank lines. For best results you should configure your editor to strip trailing whitespace on save.
  • Rather than a new TestCase subclass you can add a verifyObject/verifyClass test to the tests for the object being verified. This is technically more correct since you're not testing the interface you're testing that the class implements the interface.
  • Both IQueue and it's docstring A queue of messages are not very descriptive. IRelayQueue might be better, and the docstring could also mention that this is specifically a queue of mail messages to be relayed.
  • When describing boolean values use C{True} and C{False}
  • Typically docstring lines are ordered @param then @type
  • Docstring lines should be wrapped at column 79.
  • Queue.done appears to be public API but isn't documented on the interface.

Thanks again, I look forward to your next patch.

Changed 5 years ago by moijes12

Attachment: 4623.2.patch added

comment:10 Changed 5 years ago by moijes12

Keywords: review added
Owner: moijes12 deleted
  • C{object} is too general, if the return value is a file

object C{file} is probably appropriate. - Done this for getEnvelopeFile.

  • @return: None should be @return: C{None} - Done
  • @rtype: C{None} should be @rtype: C{NoneType} - Done
  • Your patch introduces trailing whitespace and whitespace on blank

lines. For best results you should configure your editor to strip trailing whitespace on save - Done my best at this ( Did it manually. I had some trouble when doing it with IDLE some time ago).

  • Rather than a new TestCase subclass you can add a

verifyObject/verifyClass test to the tests for the object being verified. This is technically more correct since you're not testing the interface you're testing that the class implements the interface - Done.

  • Both IQueue and it's docstring A queue of messages are not

very descriptive. IRelayQueue might be better, and the docstring could also mention that this is specifically a queue of mail messages to be relayed. - But IQueue is what the ticket wants to be ? I'm confused.

  • When describing boolean values use C{True} and C{False} - Done.
  • Typically docstring lines are ordered @param then @type - done.
  • Docstring lines should be wrapped at column 79 - Please can you guide me into doing this.
  • Queue.done appears to be public API but isn't documented on the

interface - Implemented.

comment:11 Changed 5 years ago by moijes12

Queue.done appears to be public API but isn't documented on the interface - Implemented. Should we also document addMessage ? I had built my first patch using pepijn's original patch.

comment:12 Changed 5 years ago by Tom Prince

Keywords: review removed
  • SmartHostSMTPRelayingManager is documented as taking an IQueue, which doesn't exist. The ticket documents that, but that might not in fact be the best name, as dreid suggested. Renaming it would also require changing the docstring of SmartHostSMTPRelayingManger.__init__.
  • Yes, addMessage should be documented. Similarly, the noisy}} attribute should be documented. {{{getRelayed is only used in tests, so shouldn't be part of the interface.
  • You are inconsistent about marking @return C{None on functions that have no return value. (Also, @rtype is typically omitted)
  • There should be three blank lines before and after {{{IQueue}}.
  • It would be more accurate to say (in the topfile), that the interface has been documented or created (and it should have the full path to the interface).

comment:13 Changed 5 years ago by Tom Prince

Owner: set to moijes12

(please submit for review after fixing)

Changed 5 years ago by moijes12

Attachment: 4623.3.patch added

comment:14 Changed 5 years ago by moijes12

Keywords: review added
Owner: moijes12 deleted
  • SmartHostSMTPRelayingManager is documented as taking an IQueue, which doesn't exist. The ticket documents that, but that might not in fact be the best name, as dreid suggested. Renaming it would also require changing the docstring of SmartHostSMTPRelayingManger.init. - Done
  • Yes, addMessage should be documented. Similarly, the noisy}} attribute should be documented. {{{getRelayed is only used in tests, so shouldn't be part of the interface. - addMessage is documented. I don't think documenting the noisy attribute as part of the interface is required as noisy is used for logging. I haven't documented it. getrelayed has been removed.
  • You are inconsistent about marking @return C{None on functions that have no return value. (Also, @rtype is typically omitted) - Done
  • There should be three blank lines before and after {{{IQueue}}. - Done
  • It would be more accurate to say (in the topfile), that the interface has been documented or created (and it should have the full path to the interface). - Done

comment:15 Changed 5 years ago by Tom Prince

Author: tomprince
Branch: branches/add-IRelayQueue-interface-4623

(In [37109]) Branching to add-IRelayQueue-interface-4623.

comment:16 Changed 5 years ago by Tom Prince

Keywords: review removed
  • .noisy is used by SmartHostSMTPRelayingManager (actually inside of _AttemptManager, so anything passed as a queue to it needs to have that attribute. (This may not be ideal, but it is part of the current interface).
    • An alternative would be to first change _AttemptManager to not look at that attribute (in a separate ticket) and then this could be merged without adding that attribute to the interface.
  • The docstring for createNewMessage could be imrpoved by specifying what the tuple in the return type is, e.g. `L{tuple} of L{str} and L{file}'

comment:17 Changed 4 years ago by Stacey Sern

Owner: set to Stacey Sern

comment:18 Changed 4 years ago by Stacey Sern

Ticket #6705 modifies _AttemptManager so that it does not access Queue.noisy.

Note: See TracTickets for help on using tickets.