7
\$\begingroup\$

I am relatively new to Python. I've so far mostly worked with PHP and C#, so most of it is relatively familiar.

I'm making a simple console application that will store a few API calls in a local database through a cronjob. It will have a timeout, so if the program is run too often it won't spam the API and use up all of my free access tokens.

Of course this begs for my favourite flag in any console application: --force. If something doesn't work, try forcing it, right?

Now, my first attempt at adding this was using sys.argv[n] like I would in PHP, though I read that's a rather insecure solution, which could also produce nasty bugs.

So, wanting to better my Python OOP knowledge as well as add a better implementation of CLI arguments, I've now written this:

import argparse


class Arguments:

    def __init__(self):
        self.argument_parser = argparse.ArgumentParser()
        self.argument_parser.add_argument(
            '--force',
            default=False,
            help="Force the program to run, even if the timer returns False.",
            action="store_true"
        )
        self.parsed = self.argument_parser.parse_args()

    @property
    def force_run(self):
        return self.parsed.force

Is this the right way to use classes in Python, and this is a "better" implementation of CLI arguments?

Sidenote: This was written with the assumption I'll be adding more flags in the future, all of which are accessed through the Arguments class.

\$\endgroup\$
7
\$\begingroup\$

Your code is WET. This problem will be more visible if you follow through with your sidenote:

Sidenote: This was written with the assumption I'll be adding more flags in the future, all of which are accessed through the Arguments class.

What's wrong with just using self.parsed? It gives things nice names anyway too! And so if I were to use a class I'd override __getattr__ to seamlessly use self.parsed. However this adds headaches to how to use the code, and generally isn't the best thing to do.


Why would you make the ArgumentParser every time the class is constructed? It's the same parser each time. And so I'd recommend, if you want to stick with a class, to make it a static variable. Which is bound to the class.


Combining the above could get you something like:

class Arguments:
    _argument_parser = argparse.ArgumentParser()
    _argument_parser.add_argument(
        '--force',
        default=False,
        help="Force the program to run, even if the timer returns False.",
        action="store_true"
    )

    def __init__(self):
        self._parsed = type(self)._argument_parser.parse_args()

    def __getattr__(self, name):
        return getattr(self._parsed, name)

This is problematic to use, and highlights that there should be a better way to use this. That better way would be to remove the class and just use a function. This is how I normally use ArgumentParser:

def build_parser():
    parser = argparse.ArgumentParser()
    parser.add_argument(
        '--force',
        default=False,
        help="Force the program to run, even if the timer returns False.",
        action="store_true"
    )
    return parser

if __name__ == '__main__':
    args = build_parser().parse_args()

    #...

Which is much simpler.

\$\endgroup\$
  • 1
    \$\begingroup\$ @MathiasEttinger Oh damn, you're right! I'll update the code, :) However I think it'd be 'safer' to use type(self)._argument_parser, so it's clear it's a 'class-variable' \$\endgroup\$ – Peilonrayz Sep 15 '17 at 9:00
  • \$\begingroup\$ Still seems like unneccessary overhead to me but more readable/understandable than object.__getattr__ anyway. \$\endgroup\$ – 409_Conflict Sep 15 '17 at 9:01
  • \$\begingroup\$ I would consider a writing a parse_args function as opposed to a build_parser function. One reason: the goal of main isn't to create an ArgumentParser™ object, per se, but just to parse arguments. It's a simple change and not a huge improvement, but it is slightly clearer, more re-usable, and easier to test, in my opinion. \$\endgroup\$ – jme Sep 15 '17 at 15:21
  • \$\begingroup\$ @jme Feel free to write that as an answer. I however disagree that making a function that passes args is more re-useable than something that returns an ArgumentParser. \$\endgroup\$ – Peilonrayz Sep 15 '17 at 15:30
  • \$\begingroup\$ It's such a small change, and your answer is good; I'll leave it at a comment. But the reason I call it "more re-usable" (perhaps there is a better term) is this: suppose down the line, you want to switch from using argparse to using, say, click. If I write a parse_args function, I simply need to swap out the implementation details -- but the interface of my function doesn't change. This means that all the tests I wrote for parse_args still work. But with build_parser, the interface does change -- I no longer receive an ArgumentParser. I need to change both main() and my tests. \$\endgroup\$ – jme Sep 15 '17 at 15:43
7
\$\begingroup\$

In my opinion, no, this isn't any better than just making use of the tools that argparse provides out of the box and a class is overkill. argparse.ArgumentParser operates in a very similar way, so it should be just as easy to call it straight-up:

import argparse

parser = argparse.ArgumentParser()
parser.add_argument(
    "--force",
    default=False,
    help="Force the program to run, even if the timer returns False.",
    action="store_true"
)
args = parser.parse_args()
# ...
if args.force:
    # ...
\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you for your insights! I guess classes aren't used as much in Python as they are in PHP. \$\endgroup\$ – Berry M. Sep 15 '17 at 20:16

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.