5
\$\begingroup\$

I'm just trying to learn python: I tend to write my own customized wrappers arround "more complicated" python functionalities so that i can use them more easily, more consistent, easier to change in one place, etc.

I.e. if wrote my own class my_os with i.e. has the functions rm(path) or mv(from_path, to_path) so that on application- or script-level I don't need to know or care (or remember ;)) about the specialities of os , shutil, pathlib or whatever.

I did the same for argparse. Even though my own functions are quite sparse in this case, I still prefer a named my_add_required_arg over worrying about the action parameter of the native function in every script I write.

So much to the history of this class:

#!/usr/bin/env python3
# -*- coding: utf-8 -*-

"""Wrapper functionality for Pythons built-in argparse package."""

import argparse


class myArgparser(argparse.ArgumentParser):
    """Offers a wrapper for Pythons built-in argparse package.

    It offers simple named functions to

    * add a brief description
    * set a version
    * set optional named arguments
    * set non optional named arguments
    * set optional flags

    and then to return the parsed arguments.
    """

    def __init__(self, description=None):
        """Create the main argparser object.

        Args:
            description: The optional description.
        """
        super(myArgparser, self).__init__(description=description)

        # required arguments is just printed out on "-h"
        self.myRequired = self.add_argument_group("required arguments")

    def my_set_version(self, version):
        """Set the version information, printed out by --version.

        Args:
            version: version string
        """
        self.add_argument("--version", action="version", version=version)

    def my_add_flag(self, short, long, help):
        """Add a boolean flag.

        If it is set, the flag argument is set to True, else to False.

        Args:
            short: the short name, i.e. '-d'
            long: the long name, i.e. '--debug'
            help: description printed out on '-h'
        """
        self.add_argument(short, long, help=help, action="store_true")

    def my_add_arg(self, short, long, help):
        """Add an optional named argument of string type.

        If it isn't set on the commandline, it defaults to None.

        Args:
            short: the short name, i.e. '-ld'
            long: the long name, i.e. '--logdir'
            help: description printed out on '-h'
        """
        self.add_argument(short, long, help=help, action="store")

    def my_add_required_arg(self, short, long, help):
        """Add an required named argument of string Type.

        Args:
            short: the short name, i.e. '-ld'
            long: the long name, i.e. '--logdir'
            help: description printed out on '-h'
        """
        self.myRequired.add_argument(
            short, long, help=help, action="store", required=True
        )

    def my_get_args(self):
        """Return the arguments depending on the configured and set arguments.

        Return:
            Namespace-Object containing values for all added arguments
        """
        return self.parse_args()

But using this class, I realized that I was using the same functions with oftenly the same parameters at the beginning of each of my scripts which needed arguments, so i added another function:

@classmethod
def get_args(
    cls,
    description=None,
    version=None,
    optional_args=[],
    required_args=[],
    flags=[],
):
    """Return the parsed arguments.

    Each passed argument or flag tuple must contain the short name, the long name and the description
    of the argument.

    Args:
        description: the optional description
        version: the optional version string
        optional_args: as list of tuples
        required_args: as list of tuples
        flags: as list of tuples
    """
    argparser = cls(description)

    if version is not None:
        argparser.my_set_version(version)

    for arg in optional_args:
        argparser.my_add_arg(*arg)

    for arg in required_args:
        argparser.my_add_required_arg(*arg)

    for flag in flags:
        argparser.my_add_flag(*flag)

    return argparser.my_get_args()

So now I can start a script with a clean argument definition, which I like:

# set global vars & args
version = 'mySpeedtest-20190918'
description = "Test the actual internet connection speed and log the result"
optional_args = [('-ld','--logdir', 'set destination directory for the logfile')]
flags = [('-d', '--debug', 'set loglevel to debug')]

args = my_util.my_argparse.myArgparser.get_args(description, version, optional_args, flags=flags)

Furthermore I don't have bloated init-function and still could use the single functions if I allready strongly had or wanted to.

Now the question: Does it make sense/is it good practice/is it "pythonic" to use a @classmethod as a pseudo-constructer like this?

(Kind of "Because you can do it, doesn't mean necessarily you should do it!"-Question ;))

\$\endgroup\$
  • 1
    \$\begingroup\$ Your question "Is it good practice to use a @classmethod as a pseudo-constructer ? " is a question that I feel belongs more to Software Engineering. In general Code Review will give you more advices about how to structure implementation, while SE helps more structuring interfaces. \$\endgroup\$ – Arthur Havlicek Sep 19 at 9:38
  • \$\begingroup\$ @ArthurHavlicek I guess after reading Gloweyes response, my question might not be as desired, but the response is a perfect kind of code review :) \$\endgroup\$ – evilive Sep 19 at 11:10
  • \$\begingroup\$ Just to be clear, I didn't mean that your question is offtopic there because you did post code to be reviewed and it's acceptable scopes of stack exchanges overlap a bit but if you did want a theoritical, elaborate point of view about structuring your classes API, with pro/cons of alternatives based on your requirements, then SE was more likely to give that. I actually would be surprised you don't already find good practices questions about classmethods there, if you are interested. \$\endgroup\$ – Arthur Havlicek Sep 19 at 11:38
  • \$\begingroup\$ Have you ever used the docopt library? You just write the help docstring and it automatically figures out everything from that. \$\endgroup\$ – Justin Sep 19 at 17:49
  • \$\begingroup\$ @Justin No I didn'i know it yet, thank you. Allthough it reads nice I don't think that I'm gonna use it: The helptext and parameter description printed out with -h for the user mustn't necessarily be same information as the docstring for the developer. \$\endgroup\$ – evilive Sep 20 at 5:31
2
\$\begingroup\$

I disagree on you with the need for a wrapper around the stdlib argparse, but that's a matter of taste. And I don't see anything wrong with your implementation of the rather thin wrapper.

Classmethod as Constructor

This is NOT what you're doing. A constructor creates an instance in a way somehow different from the standard and returns it. An example:

class Thing:
    def __init__(self, arg1, arg2)
        self.var1 = arg1
        self.var2 = arg2

    @classmethod
    def from_other_thing(cls, other_thing)
        return cls(other_thing.somevar, other_thing.othervar)

Your usecase

What you're doing is creating a class method that creates an instance to do work, then returning the results of that work. I'd like to emphasize that this is a perfectly valid usecase. However, your classmethod is also just about doing what you made the rest of your wrapper class for. You can easily cut down on the amount of new methods like this:

class myArgparser(argparse.ArgumentParser):
    """ Your perfectly fine docstring """

    @classmethod
    def get_args(
        cls,
        description=None,
        version=None,
        optional_args=None,
        required_args=None,
        flags=None,
    ):
        """Your other perfectly fine docstring"""
        optional_args = optional_args if optional_args is not None else []
        required_args = required_args if required_args is not None else []
        flags = flags if flags is not None else []

        argparser = cls(description)
        if version is not None:
            argparser.add_argument("--version", action="version", version=version)
        for arg in optional_args:
            argparser.add_argument(*arg, action="store")
        for arg in required_args:
            argparser.add_argument(*arg, action="store", required=True)
        for flag in flags:
            argparser.add_argument(*flag, action="store_true")
        return argparser.parse_args()

I reduced the whitespace a bit - to many newlines make reading harder. I've also changed the get_args argument to do the work you had spread over all the other 1-line functions - if a function is 1 line and used in just 1 place, then you don't really need a function for it. That's premature optimization.

Mutable Default Arguments

Don't use mutable defaults. Instead, use None and create empty lists, or change the default to an empty tuple, which is safe. I've gone for the first option, but as far as I can tell the empty tuple version would be perfectly fine as well.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much. Reading your version I had to ask myself, why I didn't do it like that "out of the box" ;) Also the reminder not to use mutable defaults is a nice one. I even read that once before, but just didn't think of it after my first test ended with the result "cannot iterate over NoneType" :D \$\endgroup\$ – evilive Sep 19 at 11:29
  • \$\begingroup\$ "Why didn't I do it like that" is a rather common thought when developing. I've had it about my own code more than I can remember. Glad to help. \$\endgroup\$ – Gloweye Sep 19 at 11:30
  • \$\begingroup\$ If you are reducing the whole class to a single classmethod, you might as well simplify it to a simple function. \$\endgroup\$ – Mathias Ettinger Sep 19 at 11:39
  • \$\begingroup\$ This class still extends the ArgumentParser class. But yes, it could also be a stand-alone function that receives the class as argument. I think both are valid choices. \$\endgroup\$ – Gloweye Sep 19 at 11:50
3
\$\begingroup\$

Using @classmethod for constructors is perfectly fine. It is basically the only reason you ever need to use them.

Classes create a namespace (and Namespaces are one honking great idea -- let's do more of those!). You don't need to preface all your methods and classes with my_. Your code becomes much more readable if you just name them according to what they do:

class ArgParser(argparse.ArgumentParser):
    ...
    def set_version(self, version):
        ...
    def add_flag(self, short, long, help):
        ...
    ...

Python also has an official style-guide, PEP8, which programmers are recommended to follow. It recommends using lower_case for methods and variables.

In general I am not a huge fan of this class. It makes building an argument parser slightly easier (nice!) at the cost of hiding away all advanced functionality (not so nice). I would try finding an interface that at least allows you to add additional options (like specifying types, defaults, nargs, etc). Maybe each argument is also allowed to be a tuple of a tuple and a dictionary (for positional and keyword arguments)?

\$\endgroup\$
  • \$\begingroup\$ Thank you for your answer. Concerning namespaces: yes you're right and in general i don't preface all my methods. But in this special case, as my Class inherits from the general ArgumentParser I just wanted to make sure not to "hide" something of it (without the need of checking if it exists or not ;)) At the moment the functionality of Argparse is totally fine for me. \$\endgroup\$ – evilive Sep 19 at 11:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.