2

As an example I have a Django custom management command which periodically (APScheduler + CronTrigger) sends tasks to Dramatiq.

Why the following code with separate functions:

def get_crontab(options):
    """Returns crontab whether from options or settings"""

    crontab = options.get("crontab")
    if crontab is None:
        if not hasattr(settings, "REMOVE_TOO_OLD_CRONTAB"):
            raise ImproperlyConfigured("Whether set settings.REMOVE_TOO_OLD_CRONTAB or use --crontab argument")
        crontab = settings.REMOVE_TOO_OLD_CRONTAB
    return crontab


def add_cron_job(scheduler: BaseScheduler, actor, crontab):
    """Adds cron job which triggers Dramatiq actor"""

    module_path = actor.fn.__module__
    actor_name = actor.fn.__name__
    trigger = CronTrigger.from_crontab(crontab)
    job_path = f"{module_path}:{actor_name}.send"
    job_name = f"{module_path}.{actor_name}"
    scheduler.add_job(job_path, trigger=trigger, name=job_name)


def run_scheduler(scheduler):
    """Runs scheduler in a blocking way"""
    def shutdown(signum, frame):
        scheduler.shutdown()

    signal.signal(signal.SIGINT, shutdown)
    signal.signal(signal.SIGTERM, shutdown)

    scheduler.start()


class Command(BaseCommand):
    help = "Periodically removes too old publications from the RSS feed"

    def add_arguments(self, parser: argparse.ArgumentParser):
        parser.add_argument("--crontab", type=str)

    def handle(self, *args, **options):
        scheduler = BlockingScheduler()
        add_cron_job(scheduler, tasks.remove_too_old_publications, get_crontab(options))
        run_scheduler(scheduler)

is better than a code with methods?

class Command(BaseCommand):
    help = "Periodically removes too old publications from the RSS feed"

    def add_arguments(self, parser: argparse.ArgumentParser):
        parser.add_argument("--crontab", type=str)

    def get_crontab(self, options):
        """Returns crontab whether from options or settings"""

        crontab = options.get("crontab")
        if crontab is None:
            if not hasattr(settings, "REMOVE_TOO_OLD_CRONTAB"):
                raise ImproperlyConfigured(
                    "Whether set settings.REMOVE_TOO_OLD_CRONTAB or use --crontab argument"
                )
            crontab = settings.REMOVE_TOO_OLD_CRONTAB
        return crontab

    def handle(self, *args, **options):
        scheduler = BlockingScheduler()
        self.add_cron_job(scheduler, tasks.remove_too_old_publications, self.get_crontab(options))
        self.run_scheduler(scheduler)

    def add_cron_job(self, scheduler: BaseScheduler, actor, crontab):
        """Adds cron job which triggers Dramatiq actor"""

        module_path = actor.fn.__module__
        actor_name = actor.fn.__name__
        trigger = CronTrigger.from_crontab(crontab)
        job_path = f"{module_path}:{actor_name}.send"
        job_name = f"{module_path}.{actor_name}"
        scheduler.add_job(job_path, trigger=trigger, name=job_name)

    def run_scheduler(self, scheduler):
        """Runs scheduler in a blocking way"""

        def shutdown(signum, frame):
            scheduler.shutdown()

        signal.signal(signal.SIGINT, shutdown)
        signal.signal(signal.SIGTERM, shutdown)

        scheduler.start()

This code is used in a one single place and will not be reused.

StackOverflow requires more details, so:

The second code is the version that I originally wrote. After that, I runned Prospector with Pylint and besides other useful messages I've got pylint: no-self-use / Method could be a function (col 4). To solve this issue I rewrote my code as in the first example. But I still don't understand why it is better this way.

Divano
  • 583
  • 5
  • 12

1 Answers1

3

At least, in this case, it is not better. Pylint is notifying you about "self" being unused, just like it would notify you about a variable or an import being unused.

Couple of other options for fixing the pylint-messages would be to actually use "self" in the functions or add staticmethod (or classmethod) decorator. Examples for both are after the horizontal line. Here are the docs for staticmethod and here's the difference between staticmethod and classmethod.

Since this is a Django-command and you likely won't have multiple instances of the class or other classes that inherit Command (that would i.e. overload the functions) or something that would benefit from the functions being inside the class, pick the one you find most readable/easiest to change.

And just for completeness, StackExchange Code Review could have further insight for which is best, if any.


Example that uses self, main difference is that scheduler is created in __init__ and not passed as an argument to the functions that use it:

class Command(BaseCommand):
    help = "Periodically removes too old publications from the RSS feed"

    def __init__(self):
        super().__init__()
        self.scheduler = BlockingScheduler()

    def add_arguments(self, parser: argparse.ArgumentParser):
        parser.add_argument("--crontab", type=str)

    def handle(self, *args, **options):
        self.add_cron_job(tasks.remove_too_old_publications, self.get_crontab(options))
        self.run_scheduler()

    # ...

    def run_scheduler(self):
        """Runs scheduler in a blocking way"""

        def shutdown(signum, frame):
            self.scheduler.shutdown()

        signal.signal(signal.SIGINT, shutdown)
        signal.signal(signal.SIGTERM, shutdown)

        self.scheduler.start()

Example that uses staticmethod, where the only difference is the staticmethod-decorator and the functions with the decorator don't have self-argument:

class Command(BaseCommand):
    help = "Periodically removes too old publications from the RSS feed"

    def add_arguments(self, parser: argparse.ArgumentParser):
        parser.add_argument("--crontab", type=str)

    def handle(self, *args, **options):
        scheduler = BlockingScheduler()
        self.add_cron_job(scheduler, tasks.remove_too_old_publications, self.get_crontab(options))
        self.run_scheduler(scheduler)

    # ...

    @staticmethod
    def run_scheduler(scheduler):
        """Runs scheduler in a blocking way"""

        def shutdown(signum, frame):
            scheduler.shutdown()

        signal.signal(signal.SIGINT, shutdown)
        signal.signal(signal.SIGTERM, shutdown)

        scheduler.start()
Toivo Mattila
  • 377
  • 1
  • 9