Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace exit(0) on error in library function with an exception #648

Merged
merged 1 commit into from
Jan 12, 2020

Conversation

huonw
Copy link
Member

@huonw huonw commented Jan 9, 2020

print + exit(0) isn't quite the right tool to indicate a failure to the user here:

  • exit(0) will result in the whole process completing immediately, even if the user had something else to do (e.g. in an interactive shell or Jupyter notebook they might pause their scripting work and download the dataset, and then come back and rerun the command/cell)
  • the 0 means it will finish as a success, rather than indicating a failure, which can mean that, for instance, testing passes accidentally because it doesn't notice that the dataset couldn't even be loaded
  • print will put output on stdout, instead of stderr, which means some tools won't pick up on it

Throwing an exception improves on all of these:

  • the exception will propagate until it is caught. For an interactive shell or Jupyter notebook, it is always caught before the program exits, so the user has the opportunity to observe the failure and then fix/rerun their code
  • if the exception isn't caught (e.g. in a non-interactive script), the process will exit with error code 1:
    $ python -c 'raise NotADirectoryError("The location {} is not a directory.".format("/some/directory"))'; echo $?
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
    NotADirectoryError: The location /some/directory is not a directory.
    1
  • the default exception printing output goes to stderr

@huonw huonw requested a review from PantelisElinas January 9, 2020 22:45
@codeclimate
Copy link

codeclimate bot commented Jan 9, 2020

Code Climate has analyzed commit 89f3cc7 and detected 0 issues on this pull request.

View more on Code Climate.

@huonw huonw merged commit ec12a97 into develop Jan 12, 2020
@huonw huonw deleted the feature/remove-exit branch January 12, 2020 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants