6
\$\begingroup\$

I'm making a little script to run on the background and notify me when I received a new email in a mailinator inbox. I want to eventually daemonise it, but for now it just runs like a regular script.

Could you give me some pointers about the code itself, the code structure and so on? I'm not sure if I should have my functions in a particular order, they're quite randomly scattered around the script at the moment.

#!/usr/bin/env python3
#encoding: utf-8

import time
import argparse
import requests

from IPython import embed as qq

try:
    from apikey import API
except ImportError:
    API = input('Please provide your API key ')

CHECKING_FREQ = 30


def parse_arguments():
    parser = argparse.ArgumentParser(description="Mailinator inbox checker")
    parser.add_argument('inbox', nargs=1, help='The inbox that you want to check')
    return parser.parse_args()


def is_new(emails):
    # Here I could probably use any() instead of next()
    # but I'm thinking I might want the email itself later on
    return next((email for email in emails
                 if email['seconds_ago'] <= CHECKING_FREQ),
                False)


def check_inbox(inbox):
    inbox_page = get_inbox(inbox)
    return inbox_page['messages']


def wait_email(inbox):
    while 1:
        emails = check_inbox(inbox)
        if is_new(emails):
            print('New email received.')
        time.sleep(CHECKING_FREQ)


def get_inbox(inbox):
    return requests.get('https://api.mailinator.com/api/inbox?'
                        'to={inbox}&token={api}'.format(
                            inbox=inbox,
                            api=API)).json()


def start_daemon():
    ...


def main():
    args = parse_arguments()
    start_daemon()
    wait_email(args.inbox[0])

if __name__ == '__main__':
    main()
\$\endgroup\$
2
\$\begingroup\$

Overall it looks great, but here are a couple of thoughts:

  • print('New email received.'): maybe print a snippet form the message or just the time stamp so you have some idea on when the new email arrived (instead of just having a stream of same strings stating "New email received.").

  • I'd consider moving the inbox operations into its own class. Its __init__ could be __init__(inbox, api): .... CHECKING_FREQ could be a keyword argument to this, defaulting to 30, but configurable through the input params when running the script. The class could have a fetch_inbox (merging your get_inbox and check_inbox). And finally the has_new_items (just rename your is_new method).

  • Your wait_email is what I'd call check_inbox because for checking the inbox you first have to fetch the inbox and then check the message timestamps on what was returned.

  • I feel someone might have a good suggestion on how to do that infinite loop, but I can't think of a good way right now. All I know is it feels a little odd (or maybe it's fine :).

Hopefully this helps.

\$\endgroup\$
1
\$\begingroup\$

I also think it looks pretty good overall. A couple things:

  • Look into logging module, once you have it running in the background (as that appears to be your plan) it will help to be able to consult a log if there are issues.
  • What happens if the "get" fails? Might want to wrap that in a try/catch, logging a message if it fails and perhaps a different logging message if there is a new mail.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.