Explicit error handling and error propagation strategy from code/dependency parsers to UI

[mknorps]

For functions that parse actual Python jupyter and dependency files, raise specialized errors and catch+continue in the caller instead of simply logger.error(...) + return in the callee.

---

Following a discussion in #97

@mknorps Why do we propagate errors with logger.error and not by raising a specialized error chain? I think we discussed it but I do not remember :( For example:

    try:
        parsed_code = ast.parse(code, filename=str(source.path))
    except SyntaxError as exc:
        logger.error(f"Could not parse code from {source}; {exc}")
        return

Why don't we propagate SyntaxError upper as InputParseError so we have explicit information at the source and leave logger out of error handling?

@jherland (https://github.com/tweag/FawltyDeps/pull/97#issuecomment-1415467013)

2. Why do we propagate errors with logger.error and not by raising a specialized error chain?

I think the most important part of this PR is that rather than aborting when we encounter errors, we instead try to keep going as far as we can. The way this is solved in this PR is mostly logger.error(...) + return, rather than raise SomeError and then try: ... except ...: in the immediate caller. I think the behavior ends up mostly being the same (i.e. what we want) in the end. Also, the resulting code is shorter when you don't have to catch the error in the caller.

However, I think it is actually strictly better to raise a specialized errors (as you suggest). Consider the following two examples, first the one where we log an error message and return:

def parse_notebook_file(path: Path) -> Iterator[ParsedImport]:
    ...
    with path.open("rb") as notebook:
        try:
            notebook_content = json.load(notebook, strict=False)
        except json.decoder.JSONDecodeError as exc:
            logger.error(f"Could not parse code from {path}; {exc}")
            return

def parse_dir(path: Path) -> Iterator[ParsedImport]:
    ...
    try:
        for file in walk_dir(path):
            if file.suffix == ".py":
                yield from parse_python_file(file)
            elif file.suffix == ".ipynb":
                yield from parse_notebook_file(file)

versus the other approach where we raised a specialized exception and then catch it:

class InputParseError(Exception):
    ...

def parse_notebook_file(path: Path) -> Iterator[ParsedImport]:
    ...
    with path.open("rb") as notebook:
        try:
            notebook_content = json.load(notebook, strict=False)
        except json.decoder.JSONDecodeError as exc:
            raise InputParseError(path) from exc

def parse_dir(path: Path) -> Iterator[ParsedImport]:
    ...
    try:
        for file in walk_dir(path):
            try:
                if file.suffix == ".py":
                    yield from parse_python_file(file)
                elif file.suffix == ".ipynb":
                    yield from parse_notebook_file(file)
            except InputParseError as exc:
                logger.error(f"Could not parse code from {exc.path}: {...}")

I think they both behave the same in this case: we log an error message and then resume parsing the next file. The second version is somewhat longer, and that could be seen as an argument against it.

But let's adopt the perspective of an (external) user of these function. If you were to reuse this code - without having a lot of experience with it - how would you expect parse_notebook_file to behave? I would argue that it is better to raise an error than to simply log an error message and return as if nothing happened.

And I think that argument is more important than trying to make the code as short as possible.

So, yes, after thinking some more about this, I agree with you @mknorps: we should rather raise specialized errors and catch+continue in the caller instead of simply logger.error(...) + return in the callee.