Template For a CLI Application

Question

I have written the following cli tool.

import sys
import argparse

parser = argparse.ArgumentParser(prog='remove-duplicate-lines-except-blank-lines',
                                 usage='%(prog)s [options] [files...]',
                                 description='Remove duplicate line except white space')
args = parser.parse_known_args()

command_line_arguments = sys.argv[1:]
lines_seen = set()  # holds lines already seen

if sys.stdin.isatty() and (len(command_line_arguments) == 0):  # has neither stdin nor file as input. Show help and exit.
    parser.print_help()
    sys.exit("Please give some input")

if len(command_line_arguments) > 1:  # 2 or more arguments, so read from file, write to file

    try:
        infile = open(sys.argv[1], "r")
    except FileNotFoundError:
        sys.exit("Input File Not Found")

    outfile = open(sys.argv[2], "w")

elif len(command_line_arguments) > 0:  # one argument
    if sys.stdin.isatty():  # one argument and stdin does not have value, so read from file and write to stdout
        try:
            infile = open(sys.argv[1], "r")
        except FileNotFoundError:
            sys.exit("Input File Not Found")
        outfile = sys.stdout
    else:  # one argument and stdin has value, so read from stdin and write to file
        infile = sys.stdin
        outfile = open(sys.argv[1], "w")

else:  # no arguments, so read from stdin and write to stdout
    infile = sys.stdin
    outfile = sys.stdout

for line in infile:
    if line not in lines_seen:
        outfile.write(line)
        if line != "\n":
            lines_seen.add(line)

if infile is not sys.stdin:
    infile.close()

if outfile is not sys.stdin:
    outfile.close()

The purpose of this tool is to remove duplicate lines (except blank lines). It ignores blank lines.

It has --help and -h to get help.

The logic here is, it takes input from stdin or input-file. It gives output to stdout or output-file. So, total four combinations.

• input from stdin and output to stdout
• input from stdin and output to output-file
• input from input-file and output to stdout
• input from input-file and output to output-file

There is a special case where there is no input (neither from stdin nor from input-file). Which is covered using:

if sys.stdin.isatty() and (len(command_line_arguments) == 0):  # has neither stdin nor input file
    parser.print_help()
    sys.exit("Please give some input")

The problem with this code is, the logic seems over complicated. too many conditions giving me the sense that something is wrong here. However, because of these conditions, it has become very hard to test the code.

Another problem with the code is the help, which looks like:

usage: remove-duplicate-lines-except-blank-lines [options] [files...]

Remove duplicate line except white space

optional arguments:
  -h, --help  show this help message and exit

I am also looking for feedback on how to make the help option better. Given few things are not clear (mentioned bellow in bullet point) just by looking at usage: remove-duplicate-lines-except-blank-lines [options] [files...]. I am also not clear on how to write the usage properly.

• When there is stdin and one file as argument, the file is considered as output-file.
• When there is no stdin and one file as argument, the file is considered as input-file, and the output goes to the stdout.

I want to keep this piece of code as a template for future cli apps written in python. So, your critique is very important for me.

Answer 1

A few comments in no particular order:

• When reading, you are doing just fine, as you are processing one line at a time. However, you are storing the lines you read in memory. This could cause the program using lots of memory if reading long files.

This has a simple solution as you don't really need to keep the lines in memory. All you really want is knowing whether a line has been seen before. To do that, simply hash the lines.

• As you have already stated in your concerns about how to write the help message, it is kind of hard to explain what your code does.That's simply because I find the core logic of argument parsing is kind of difficult.

Just have a look at the cat manual: it's really simple and intuitive, if any arguments are given, they are input files to be read. Otherwise, just read from stdin. Then, if you want to write to a file, use the already existent > syntax to write stdout to a file.

Maybe you could change your code logic to be like that.

• When writing a CLI, I'd recommend you to use an already existing library, like click or fire (I personally like fire better, as there is less code to write). That hides all the complexity of parsing arguments under the library, and forces you to better organise your code by creating functions.

• Your handling of files is not taking advantage of the with statement (due to not having extracted the core logic of your code of removing repeated lines into a function). This makes you have to manually close the input and output files, which isn't fail-safe: if you have any exception, the file closing will not be controlled. Also, you will close stdin or stdout in some cases, which seems a bit nasty (I'm not sure if the same applies to python, but here is a question about closing stdin in C).

Edit

Here is a sample code of how you could update it:

from pathlib import Path
from typing import TextIO
import sys

import fire

def _remove_duplicates(input_file: TextIO, output_file: TextIO) -> None:
    found_lines_hash = set()
    for line in input_file:
        line_hash = hash(line)
        if not line_hash in found_lines_hash:
            if line != "\n":
                found_lines_hash.add(line_hash)
            output_file.write(line)

def remove_duplicates(input_file_path: str) -> None:
    input_file_path = Path(input_file_path)
    if not input_file_path.exists():
        print(f"File does not exist '{input_file_path}'")
        return
    with input_file_path.open() as input_file:
        _remove_duplicates(input_file, sys.stdout)

if __name__ == "__main__":
    fire.Fire(remove_duplicates)

Then, you would run it like:

$ python remove_duplicates.py /path/to/my/input/file > /path/to/output

To handle the stdin case you could add another function that also calls _remove_duplicates.

I have to say I haven't been able to run the code, as I have written it from my phone. But you get the idea

Answer 2

The absence of declared functions is a clear indication of how you can better organize your code.
Also, you're using argparse, which is good, but you're bypassing it in several ways, which isn't.

Regarding argparse:

• Don't write your own usage if you can help it; let the parser write it based on it's own configuration. If you need to add additional notes, here's some copypasta from a tool of my own to "have your cake and eat it":

def removeprefix(s, p):  -- only needed for python versions <3.9
    return s[len(p):] if s.startswith(p) else s

parser.usage = (
    removeprefix(parser.format_usage(), 'usage: ')
    + "  (... my notes to users ...)"
)

• Check out the examples in the docs for the nargs option. Specifically I think you'll be able to use some combination of '?', '*', '+', and argparse.REMAINDER to bind the input and output files.
• It's nice to give all your arguments optional names, so if a user wants to be explicit they can.
• Check out FileType args! I've never used them, but they look pretty cool! (It's unclear how/if you're supposed to close them though?)

Other stuff:

• Do have a core function that takes an open "file" to read from and an open file to write to.
• Why are you checking sys.stdin.isatty()? The comment suggests you think it's checking if stdin is available, but it's actually checking for an interactive shell. I see no reason you shouldn't be able to pipe streams into this program.
• What if I want to read from several files in sequence, removing duplicate lines from the whole batch? One could write cat A.txt B.txt C.txt | rdlebl > D.txt (and relying on piping like this is a valid option!), but if your program is going to have built-in file management then it might as well be able to handle multiple files.
• Use with blocks to manage open files.
• What about non-newline whitespace? If a line is " \n", should it be treated as empty?

Edit

Since m-alorda added a code, I stopped resisting the urge to write my own. It was fun; I hope it's useful!

#!/usr/bin/env python3

from argparse import ArgumentParser, FileType, REMAINDER
from itertools import chain
from sys import stdin

# Not Implemented:
# Whitespace options
# Text-encoding options
# [Don't] clobber options
# Output as a positional argument

def main():
    args = get_args()
    deduper = Deduper()
    # I didn't see a way for argparse to have a default value
    #  for args that merge, but maybe I missed something.
    sources = chain.from_iterable(args.source) if args.source else stdin
    for line in deduper.dedup(sources):
        args.out.write(line)

# If you think inheriting from set is sketchy, wrap set in a dataclass.
class Deduper(set):
    def dedup(self, source):
        for line in source:
            # this behavior isn't exactly the same as yours.
            stripped = line.strip()
            if stripped:
                if stripped not in self:
                    yield line
                    self.add(stripped)
            else:
                yield '\n'

def get_args():
    arg_parser = ArgumentParser(description="Remove duplicate lines.")
    arg_parser.add_argument('-o', '--out',
                            default='-',
                            type=FileType(mode='w'),
                            metavar='FILE',
                            help="The file to write to, defaults to stdout.")
    arg_parser.add_argument('-f', '--file',
                            dest='source',
                            action='append',
                            type=FileType(mode='r'),
                            metavar='FILE',
                            help="A file to read from, or - for stdin.")
    arg_parser.add_argument('source',
                            action='extend',  # assumes python version >=3.8
                            nargs='*',
                            type=FileType(mode='r'),
                            metavar='FILE',
                            help="The file(s) to read from, defaults to stdin.")
    return arg_parser.parse_args()

if __name__ == '__main__':
    main()
```

Answer 3

OP here, the rewrite of the program looks like:

import sys
import argparse

lines_seen = set()  # holds lines already seen

parser = argparse.ArgumentParser()
parser.add_argument('infile', nargs='?', type=argparse.FileType('r'), default=sys.stdin)
args = parser.parse_args()

# taking input from stdin which is empty, so there is neither a stdin nor a file as argument
if sys.stdin.isatty() and args.infile.name == "<stdin>":
    sys.exit("Please give some input")

for line in args.infile:
    if line not in lines_seen:
        print(line)
    if line != "\n":
        lines_seen.add(line)

Here, you can see that I am not taking output-file as argument at all. This is because, by using > we can redirect the program's output anyways. So, taking output as argument just increase the complexity of the program.