PR#152: Supress tracebacks in more cases - fedora-rust/rust2rpm

fedora-rust / rust2rpm

#152 Supress tracebacks in more cases

Merged 2 years ago by zbyszek. Opened 2 years ago by zbyszek.

fedora-rust/ zbyszek/rust2rpm catch-more-errors into main

cargo-inspector: don't print the traceback if a file is missing

Zbigniew Jędrzejewski-Szmek • 2 years ago

7b85307

Supress tracebacks for "expected" exceptions in more places

Zbigniew Jędrzejewski-Szmek • 2 years ago

aa849b8

rust2rpm/__main__.py

file modified

+18 -7

		`@@ -299,6 +299,22 @@`
		`autochangelog_re = r"^\s*%(?:autochangelog\|\{\??autochangelog\})\b"`
		`return any(re.match(autochangelog_re, line) for line in text.splitlines())`

		`+`
		`+ @contextlib.contextmanager`
		`+ def exit_on_common_errors():`
		`+ """Suppress tracebacks on common "expected" exceptions"""`
		`+ try:`
		`+ yield`
		`+ except requests.exceptions.HTTPError as e:`
		`+ sys.exit(f'Failed to download metadata: {e}')`
		`+ except subprocess.CalledProcessError as e:`
		`+ cmd = shlex.join(e.cmd)`
		`+ sys.exit(f'Subcommand failed with code {e.returncode}: {cmd}')`
		`+ except FileNotFoundError as e:`
		`+ sys.exit(str(e))`
		`+`
		`+`
		`+ @exit_on_common_errors()`
		`def main():`
		`default_target = get_default_target()`

		`@@ -473,11 +489,6 @@`
		`with open(patch_file, "w") as fobj:`
		`fobj.writelines(diff)`

		`+`
		`if __name__ == "__main__":`
		`- try:`
		`- main()`
		`- except requests.exceptions.HTTPError as e:`
		`- sys.exit(f'Failed to download metadata: {e}')`
		`- except subprocess.CalledProcessError as e:`
		`- cmd = shlex.join(e.cmd)`
		`- sys.exit(f'Subcommand failed with code {e.returncode}: {cmd}')`
		`+ main()`

rust2rpm/inspector.py

file modified

		`@@ -5,6 +5,7 @@`

		`from . import Metadata`
		`from .metadata import normalize_deps, Dependency`
		`+ from .__main__ import exit_on_common_errors`


		`def _get_binaries(cargo_toml):`
		`@@ -49,6 +50,7 @@`
		`raise FileNotFoundError(f'Cargo.toml not found for binary {binary_or_cargo_toml}')`


		`+ @exit_on_common_errors()`
		`def main():`
		`parser = argparse.ArgumentParser()`
		`group = parser.add_mutually_exclusive_group(required=True)`

no initial comment

decathorpe commented 2 years ago

There's some things I don't like about your approach:

it's very "magical" by using decorators and syntax that I don't even know after years of using Python
catching exceptions happens just by wrapping the main function, not where they are actually thrown (this is usually a bad code smell)
the decorator is defined in one program's "main" file and is imported into another program's "main" file - it would make more sense to define the decorator in some common file instead of having to import another file that's intended to just be a script (which might have all sorts of side effects)

zbyszek commented 2 years ago

Oh, I missed your comment. Thanks for looking.

it's very "magical" by using decorators and syntax that I don't even know after years of using Python

Hmm, it's using the bog-standard way of defining context managers. I think the code is as simple as can be.

catching exceptions happens just by wrapping the main function, not where they are actually thrown (this is usually a bad code smell)

The whole point of exceptions is that they can be caught far from the origin point. This allows the handling of errors to be separated from the normal function call flow. In this case, we are talking about fatal exceptions like network errors, so the inner functions have no way of handling them. The inner functions do not and cannot know how to report the exception, and the only thing they can reasonably do is to propagate it to the caller. And all the caller does is that it decides that for certain types of exceptions a traceback should not be printed, and a single custom error message is enough.

I'm not sure how you'd want to structure the code instead, but I'm pretty sure that letting the inner functions decide how to report the error would be a very bad idea.

the decorator is defined in one program's "main" file and is imported into another program's "main" file - it would make more sense to define the decorator in some common file

I agree it'd be better to have it in some other file, but there is no file like that yet. Maybe it'd make sense to create one with more stuff, but I'm leaving that for some future refactoring.

instead of having to import another file that's intended to just be a script (which might have all sorts of side effects)

The file uses if __name__ == '__main__', so it'd only have side effects if we screwed this up. Bugs are always possible, but this doesn't seem a likely issue.

OK, I'll merge this as is. I fixes the issue nicely, and it just seems we like different programming styles.