3
\$\begingroup\$

I use this code to check that all files in a directory are free from Unicode and non-printable characters.

Can I improve the structure of the code?

#!/usr/bin/env python3

import argparse
import os
import string
import sys

def main(input_folder):
    for directory, subdirs, files in os.walk(input_folder):
        for basename in files:
            path = os.path.join(directory, basename)
            try:
                check_file(path)
            except (ValueError, UnicodeDecodeError) as e:
                print(e)
                sys.exit(1)
    print('All files are ok!')


def check_file(path):
    with open(path, encoding='utf-8') as fp:
        try:
            data = fp.read()
        except UnicodeDecodeError:
            raise ValueError('Warning! {} contains non ascii characters'.format(path))
        if not is_printable(data):
            raise ValueError('Warning! {} contains non printable characters'.format(path))


def is_printable(s):
    return all(c in string.printable for c in s)


if __name__ == '__main__':
    parser = argparse.ArgumentParser(description='Verify correctness of files.')
    parser.add_argument('--input', required=True, help='Input folder')
    args = parser.parse_args()
    main(args.input)
\$\endgroup\$
2
\$\begingroup\$

Your problem specification is unclear, and the code may be wrong as a result. What exactly do you mean by "Unicode characters"? What if a file contains just two bytes, 0x20 and 0x5D — is that two ASCII characters (a Space and a Right Square Bracket ]), or is that one UTF-16 character (U+205D Tricolon )?

Unless you have a clear definition of what you mean by "Unicode", and have a special reason for detecting non-ASCII characters specifically, you may be better off opening the file with a latin_1 encoding and just checking is_printable(). In UTF-8, any multibyte sequence will have the leading bit set in all but the last byte, which would place it outside the string.printable range. You would reject the same files as before, just with a less specific reason given.

For scalability, it would be better to read fixed-size blocks (perhaps around 8 kB) instead of the entire file at once. It might also be more efficient to check c in set(string.printable) rather than c in string.printable.

\$\endgroup\$
0
\$\begingroup\$

Overall, it's a nice script. Some minor improvements are possible.


main is not a very good name for a method that checks the files in a directory tree. The code in the if __name__ == ... guard pollutes the global scope. The solution is simple: rename main to for example check_dir_tree, and move the code in the if __name__ == ... guard inside main. That way, the main function will be in charge of the argument parsing, which is a perfectly reasonable responsibility for a function with this name.

That is, like this:

def check_dir_tree(input_folder):
    for directory, subdirs, files in os.walk(input_folder):
        for basename in files:
            path = os.path.join(directory, basename)
            try:
                check_file(path)
            except (ValueError, UnicodeDecodeError) as e:
                print(e)
                sys.exit(1)
    print('All files are ok!')


def main():
    parser = argparse.ArgumentParser(description='Verify correctness of files.')
    parser.add_argument('--input', required=True, help='Input folder')
    args = parser.parse_args()
    check_dir_tree(args.input)


if __name__ == '__main__':
    main()

It seems to me that the check_file function will never actually raise UnicodeDecodeError. So the main function should not try to catch that, only ValueError.


ValueError is too generic. It might be a good idea to create a custom exception class for your purposes.

\$\endgroup\$

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.