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()

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}. — Peilonrayz
– Peilonrayz ♦, Commented Jun 11, 2019 at 14:35

Peilonrayz · Accepted Answer · 2019-06-11 15:01:40Z

Return early, there's no need to touch files if the current file is <5mb.
DRY your code, move the "{}{}.{}".format into it's own function.
Don't use comprehensions for mutations. Use an explicit for loop for that.
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.
You can use the pairwise recipe to make the reversed pairwise loop. I think it's more readable, however you may not.
It's unidiomatic to put brackets on if statements. Unless the statement spans multiple lines.
It's idiomatic to put two newlines around top level classes and functions.
I'm a bit confused why you want to touch everything.
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()

"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()… — 301_Moved_Permanently
– 301_Moved_Permanently, Commented Jun 11, 2019 at 15:17
@MathiasEttinger That's a good point. It makes sense if it's renamed to make_empty. — Peilonrayz
– Peilonrayz ♦, Commented Jun 11, 2019 at 15:24
Also, 10. Using os.rename instead of shutil.copyfile since you don't need copies anyway. — 301_Moved_Permanently
– 301_Moved_Permanently, Commented Jun 11, 2019 at 15:28
@MathiasEttinger Do you want to post an answer? Even if you copied your existing comments I'd upvote it :) — Peilonrayz
– Peilonrayz ♦, Commented Jun 11, 2019 at 15:30
No, I shouldn't even be here now, I’m a bit in a hurry now ;) — 301_Moved_Permanently
– 301_Moved_Permanently, Commented Jun 11, 2019 at 15:31

Stack Exchange Network

Simple log rotation script

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Simple log rotation script

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions