trjhtr

I am brand new to Python coming from Ruby on Rails. I am building a cleanup script for cleaning (archiving, deleting, or viewing) directories. I would love to know anything that I can make better, safer, faster.

import os
import re
import shutil
import argparse
from datetime import datetime

# Example: python cleanup.py -d u:jobs -p u:jobspattern_file.txt -r list
"""
Example pattern file:

template
^d5,
twin

"""
parser = argparse.ArgumentParser()
parser.add_argument('-p', type=str, default="pattern_file.txt", help="Pattern File (One item per line, regex or string)")
parser.add_argument('-d', help='Directory to clean up.')
parser.add_argument('-a', help='Minimum number of days old to clean up.')
parser.add_argument('-r', type=str, help='delete, archive or test.')
args = parser.parse_args()


def included_files(directory, pattern_file, age_string = 90):
 """ Find files to move """
 all_files = 
 excluded_files = 

 os.chdir(directory)

 patterns = fix_pattern(pattern_file)
 for fname in os.listdir(directory):
 all_files.append(fname)

 for pattern in patterns:

 # Check for matching pattern in filename
 if re.search(pattern, fname.lower()): # Regex search
 excluded_files.append(fname)

 # Check to see if the difference in today and modified date is more than "age"
 now = datetime.now()
 then = datetime.fromtimestamp(os.path.getmtime(fname))
 tdelta = now - then
 if type(age_string) == str:
 age = int(age_string)
 else:
 age = 90
 if tdelta.days < age:
 excluded_files.append(fname)
 value = [x for x in all_files if x not in excluded_files]
 return value


def file_maintenance(dir_path, files, type):
 """ File Maintenance """
 if os.path.isdir(dir_path):
 directory = dir_path
 else:
 directory = os.path.dirname(os.path.realpath(__file__))
 archive_directory = os.path.join(directory.strip(), 'archive', "")

 for file in files:
 if os.path.isdir(file) is not True:
 if type is "archive":
 if os.path.isdir(archive_directory) is not True:
 os.makedirs(archive_directory)
 print("Moving " + os.path.join(directory, file) + " to " + os.path.join(archive_directory, file))
 shutil.move(os.path.join(directory, file), os.path.join(archive_directory, file))
 elif type is "delete":
 print('Deleting ' + os.path.join(directory, file))
 os.remove(file)
 else:
 print("File: " + os.path.join(directory, file))


def fix_pattern(pattern_file):
 if os.path.isfile(pattern_file):
 print("pattern file found")
 with open(pattern_file) as f:
 content = f.readlines()
 pattern = [x.strip() for x in content]

 else:
 print("pattern file not found")
 pattern = 
 pattern.append('pattern_file')
 return pattern


if __name__ == "__main__":

 files = included_files(args.d, args.p, args.a)
 file_maintenance(args.d, files, args.r)

I would do the following:

• Use PyCharm or IntelliJ IDEA with the Python plugin. Either will detect several of the following issues, and you can simply cycle through them using F2.


• Put the argument parsing inside a function. It's clearer and you avoid issues with importing your stuff from other code.


• Run the code through pycodestyle to discover several issues such as foo is not True which should be not foo.


• Use long name options such as --pattern-file to make your options obvious to a new user.


• Always treat age_string as an int. This means setting the ArgumentParser argument type, renaming the parameter and removing all the if type(age_string) == str stuff.


• Inline the value variable - it's only used once, and that in a trivial manner.


• Use a single list rather than adding all the files to a list, then creating a list of exclusions, then subtracting one from the other. Do all the checks and continue the loop if any of them apply to the file under consideration.


• Create constants for any magic values, for example archive and 90.


• Understand why the string pattern_file is added to the pattern list, and most likely remove it.


• Make it obvious (in the code, not just the argument help text) that the patterns are lists of exclusions.
  


• In general, don't try to second guess the user's intentions. For example, it should be an error if you have a dir_path parameter which does not point to an existing directory. If it points to a file then the user has misunderstood how to use it.