Simple log rotation script

Question

I created a simple script that implements log rotation. It checks the log file size (say, out.log) if it exceeds 5MB limit. If it does then the script copies it to out1.log and clears out.log. But it also copies out1.log to out2.log, etc. So log files are kind of 'shifted' and contents of the last one is discarded. In this implementation I keep only 5 files (initial out.log and out[1-4].log).

I am not a Python programmer, although I find this language extremely useful for such kinds of tasks. I would like my code to be as much idiomatic as possible, so what can I change/improve in my script for this?

from os import listdir
from os.path import getsize, exists
from shutil import copyfile
from argparse import ArgumentParser
SIZE_5MB = 5e6
MAX_LOG_FILES_COUNT = 5
class LogRotator(object):
 def __init__(self, prefix, suffix):
 self.prefix = prefix
 self.suffix = suffix
 def __str__(self):
 return "{}[x].{}".format(self.suffix, self.prefix)
 def __touch(self, file_name):
 open(file_name, 'w').close()
 def rotate(self):
 files = ["{}{}.{}".format(self.prefix, x + 1, self.suffix) for x in range(MAX_LOG_FILES_COUNT)]
 [self.__touch(f) for f in files if not exists(f)]
 current_log = "{}.{}".format(self.prefix, self.suffix)
 if (getsize(current_log) < SIZE_5MB):
 return
 files.insert(0, current_log)
 for i in range(len(files) - 1, 0, -1):
 copyfile(files[i-1], files[i])
 self.__touch(files[0])
if __name__ == '__main__':
 parser = ArgumentParser(description="Rotate log files")
 parser.add_argument("-prefix", help="log file prefix")
 parser.add_argument("-suffix", help="log file suffix")
 args = parser.parse_args()
 logRotator = LogRotator(args.prefix, args.suffix)
 logRotator.rotate()

• Peilonrayz: Could you add a short description on what this does. From my understanding of your code it finds a log that is <5mb and overwrites all files with {self.prefix}5.{suffix}.
• Dmitry Zinkevich: @Peilonrayz yes, sure

Answer 1

1. Return early, there's no need to touch files if the current file is <5mb.
2. DRY your code, move the "{}{}.{}".format into it's own function.
3. Don't use comprehensions for mutations. Use an explicit for loop for that.

4. As you want idiomatic code, you should be aware that some people discourage the use of __ as it performs name mangling.

   If the function is intended to be private, not protected, then it should be ok.

5. You can use the pairwise recipe to make the reversed pairwise loop. I think it's more readable, however you may not.
6. It's unidiomatic to put brackets on if statements. Unless the statement spans multiple lines.
7. It's idiomatic to put two newlines around top level classes and functions.
8. I'm a bit confused why you want to touch everything.
9. You may want to split all the different aspects of rotate into smaller functions to follow SRP.

Your code's pretty idiomatic otherwise, your naming conventions are good, you've used a main guard and you've got white space where it should be.

from os import listdir
from os.path import getsize, exists
from shutil import copyfile
from argparse import ArgumentParser
import itertools
def pairwise(iterable):
 "s -> (s0,s1), (s1,s2), (s2, s3), ..."
 a, b = itertools.tee(iterable)
 next(b, None)
 return zip(a, b)
SIZE_5MB = 5e6
MAX_LOG_FILES_COUNT = 5
class LogRotator(object):
 def __init__(self, prefix, suffix):
 self.prefix = prefix
 self.suffix = suffix
 def __str__(self):
 return "{}[x].{}".format(self.suffix, self.prefix)
 def __touch(self, file_name):
 open(file_name, 'w').close()
 def _gen_file_name(self, name):
 return "{}{}.{}".format(self.prefix, name, self.suffix)
 def rotate(self):
 current_log = self._gen_file_name('')
 if getsize(current_log) < SIZE_5MB:
 return
 files = [
 self._gen_file_name(i)
 for i in range(1, MAX_LOG_FILES_COUNT + 1)
 ]
 for file in files:
 if not exests(file):
 self.__touch(f)
 for older, newer in pairwise(reversed([current_log] + files)):
 copyfile(newer, older)
 self.__touch(current_log)
if __name__ == '__main__':
 parser = ArgumentParser(description="Rotate log files")
 parser.add_argument("-prefix", help="log file prefix")
 parser.add_argument("-suffix", help="log file suffix")
 args = parser.parse_args()
 log_rotator = LogRotator(args.prefix, args.suffix)
 log_rotator.rotate()

• 301_Moved_Permanently: "I'm a bit confused why you want to touch everything." => I’m even more confused as it not only touch, but also clears the files. A simple touch would be open(file_name, 'a').close()...
• Peilonrayz: @MathiasEttinger That's a good point. It makes sense if it's renamed to make_empty.
• 301_Moved_Permanently: Also, 10. Using os.rename instead of shutil.copyfile since you don't need copies anyway.
• Peilonrayz: @MathiasEttinger Do you want to post an answer? Even if you copied your existing comments I'd upvote it :)
• 301_Moved_Permanently: No, I shouldn't even be here now, I’m a bit in a hurry now ;)