2
\$\begingroup\$

I wrote this to check files for successful compression with LZ4.

Any advice is welcome.

Particularly regarding raising exceptions and error handling.

import sys

def check_lz4_header(file_path):
    with open(file_path, 'rb') as file:
        header = file.read(4)  # Read the first 4 bytes (typical size of LZ4 header)
        if header != b'\x04\x22\x4d\x18':  # Check for LZ4 magic number (optional)
            raise ValueError("Invalid header: This does not appear to be an LZ4 compressed file.")
        else:
            print("This appears to be an LZ4 compressed file.")

# If the script is run as a standalone script, execute the check_lz4_header function
if __name__ == "__main__":
    if len(sys.argv) != 2:
        print("Usage: python lz4_header_checker.py <file_path>", file=sys.stderr)
        sys.exit(1)
    
    file_path = sys.argv[1]
    try:
        check_lz4_header(file_path)
    except ValueError as e:
        print(e, file=sys.stderr)
        sys.exit(1)
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Since this is a script, it's important that you add a hashbang #!.

Since you check for the header, throwing and immediately catching, there's no meaningful purpose to stack unwinding. You can convert the check function to just return a boolean.

It's not much more code to use argparse properly, so you might as well. Bonus: it can construct a Path object for you.

Don't put your code in the global namespace (code in a __name__ guard is still in the global namespace).

Typical of Unix utilities is, on success, to print nothing. That's probably a good idea here.

In your check function, the moment you have your header, you should leave the file's context management scope.

All together,

#!/usr/bin/env python3

import argparse
import sys
from pathlib import Path


def is_lz4(file_path: Path) -> bool:
    with file_path.open('rb') as file:
        header = file.read(4)
    return header == b'\x04\x22\x4d\x18'


def parse_args() -> argparse.Namespace:
    parser = argparse.ArgumentParser(
        description='Return 0 if the given file has an LZ4 header',
    )
    parser.add_argument('file_path', type=Path)
    return parser.parse_args()


def main() -> None:
    args = parse_args()
    try:
        if not is_lz4(args.file_path):
            print(
                'Invalid header: This does not appear to be an LZ4-compressed file.',
                file=sys.stderr,
            )
            exit(1)
    except IOError as e:
        print(e, file=sys.stderr)
        exit(1)


if __name__ == '__main__':
    main()
\$\endgroup\$
1
\$\begingroup\$

Usage of the word "optional" in the comment in the following line is unclear:

    if header != b'\x04\x22\x4d\x18':  # Check for LZ4 magic number (optional)

If you mean that the first 4 bytes in an LZ4 file may or may not correspond to that magic number, it would be better to clarify that in the comment. If you mean something else, you should clarify that as well. Otherwise, just delete the "(optional)".

I find code easier to understand when the if clause in an if/else uses an == instead of != (or any "negative" logic):

    if header == b'\x04\x22\x4d\x18':  # Check for LZ4 magic number (optional)
        print("This appears to be an LZ4 compressed file.")
    else:
        raise ValueError("Invalid header: This does not appear to be an LZ4 compressed file.")

Consider printing the file name (file_path) with the messages in the function. This will be useful information for the user, especially if you are calling the function several places in larger code.

Otherwise, the code is clear and easy to understand. You are consistent in your use of stderr for error messages and stdout for ordinary messages.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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