search

wking / rss2email

open-source tool for Windows, Mac OS and UNIX for getting news from RSS feeds in email
http://pypi.python.org/pypi/rss2email/
GNU General Public License v2.0
270 stars 48 forks source link

small fixes for using imap as a backend #4

Closed arunpersaud closed 11 years ago

arunpersaud commented 11 years ago

Signed-off-by: Arun Persaud apersaud@lbl.gov

wking commented 11 years ago

On Fri, Apr 05, 2013 at 12:43:03PM -0700, arunpersaud wrote:

  • small fixes for using imap as a backend

Looks good, but the commit message is a bit terse ;). It would be nice to have some of the comments you attached to your original diff:

Fri, Apr 05, 2013 at 10:48:03AM -0700, Arun Persaud:

I had to add some small modifications tothe code, basically two typos in def send and some of the imap connection commands were not needed (tested only with gmail, not sure if it's different for other imap interfaces):

  • imap.connect is not needed
  • imap.close is only needed after you selected a mailbox (I think) which doesn't happen, so it gave an error.

Comments like these are useful when we're looking back at this change later and trying to figure out why the imap.connect() call was removed ;). You should be able to fix your comment with:

$ git checkout imap $ git commit --amend $ git push --force arunpersaud imap

I usually prefix the summary line with the affected module:

email: Small fixes for using IMAP as a backend

which is style similar to that used by Linux and related projects 1.

Thanks, Trevor

This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

wking commented 11 years ago

Hi

On 04/05/2013 01:07 PM, W. Trevor King wrote:> [...]

Comments like these are useful when we're looking back at this change later and trying to figure out why the imap.connect() call was removed ;). [...]

done... I guess the original pull request is still valid or do I need to submit a new one?

ARUN

wking commented 11 years ago

On Fri, Apr 05, 2013 at 01:12:24PM -0700, Arun Persaud wrote:

done...

Looks good to me.

I guess the original pull request is still valid or do I need to submit a new one?

It's still valid. When you push to your branch, GitHub updates the PR to point to your new branch tip. Not that this really matters here, since I'll just merge the imap branch directly from your GitHub repository ;). Actually, my initial commit message was also a bit terse, so I fleshed it out and then cherry picked your commit onto my new commit. I'll let the new imap branch cook in my public repositories for a bit to collect additional feedback.

Cheers, Trevor

This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

wking commented 11 years ago

This PR has been merged into imap (rebased as a1c7677a1139fb95af89b3cc8aef0eb24207d42f).