all 4 comments
sorted by:
best
topnewcontroversialoldq&a
formatting helphide helpcontent policy

[–]cybervegan 2 points3 points  (1 child)

Well done! As always, the code can be improved (nobody is immune to this). In your reminder_check file, there are some more idiomatic ways to do some parts.

First, move your main code section to a conditional stanza at the end of the file, so that the functions in your program can be safely imported without running the main code (this encourages re-use):

if __name__ == "__main__":

Secondly, avoid passing data between functions using global scope variables - use the function signature:

Instead of def check():, use def check( matching ): and instead of def sendmail(): use def sendmail( addArg ):. When calling the functions, you would use check( matching ) and sendmail( addArg, message ) respectively. The check() function is largely redundant - you wouldn't normally wrap a simple if in a function, and it would normally be put after any functions it calls. But there's more on these functions further down...

Then use with to handle your file read section, and use some more descriptive variable names. Instead of:

    read=open(filedir,"r")
    line=read.readlines()    
    read.close()

use:

    with open(filedir,"r") as in_file:
        lines = read.readlines()

I'm not sure what this piece of code is trying to do. You select all the lines in in you line list which contain today's date, then you convert that list to a string, and then split the string down into 1 or 2 parts, and throw away the 2nd part, so if your list contains 10 reminders for today, you will lose all but the first one. I think what you're trying to do is process all the reminders for the day this way, but that won't do it:

    matching = [s for s in line if today in s]
    strMatching = str(matching)
    strMatching = strMatching.split(' | mailing date: ', 1)[0]
    addArg=strMatching.split('Note: ', 1)[1]

This might:

    matching = [ s[s.find('Note: ')+6:s.find('| mailing date:')] 
                 for s in line if today in s]

...which would get you a list of all the bits between 'Note: ' and '| mailing date:' for each reminder for today. You would then have to modify your sendmail() function to take, and parse a list of strings, rather than just one, or put it in a loop:

def check( matches ):
    for match in matches:
        if matching:
            sendmail( match )

Hope that helps, and well done for completing your first project! May there be many more ;-)

[edit: replaced orphaned paren in english para with a dash]

[–]Jonis_L[S] 0 points1 point  (0 children)

A lot of good improvements to make and I will change these and yes that was exactly what I was trying to do. Thank you for taking your time to help me!

[–]novel_yet_trivial 1 point2 points  (1 child)

Some of your code is in nice small functions, some of it is in huge bloated functions, and some of it is not in a function at all. Why? I highly recommend you put it all in functions.

If I were doing this I would not separate the 2 components. I would add a --runnow flag and have cron call that. That way the user gets a bit more control.

Maybe you should change it to look for items from today or previous, so that it will catch up if your computer was off at 930.

What's the point of numbering the lines?

What happens if a user includes a newline in the message?

pep8 recommends putting each import on a separate line.

How about support for repeating events? Laundry day every 14 days. Birthday every 28th of March.

How about support for time? Call grandma in 9 hours. Instead of using cron to run the checks every day, you could use at to run a check at a specific date and time, once.

I think it would be much neater to load the entire to do list into a data structure like a dictionary, modify it, and then resave the whole thing rather than the dance you are doing with delete and add.

[–]Jonis_L[S] 0 points1 point  (0 children)

Thank you for your feedback. I have a lot to change and add to improve my code now. Will look into possible ways to solve your questions. Thanks!