Replace sys.exit calls with exceptions #318
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facundobatista
requested changes
May 1, 2018
fades/envbuilder.py
Outdated
| class FadesError(Exception): | ||
| """Provides a Fades exception.""" | ||
|
|
||
| def __init__(self, reason): |
There was a problem hiding this comment.
This is not really needed, as Exceptions already receive and store a message, so just define FadesError, with the docstring, and that's all.
That said, it's better to define this exception in a more general place... put it in fades/__init__.py and import it from there...
fades/helpers.py
Outdated
| @@ -135,7 +135,7 @@ def _get_interpreter_info(interpreter=None): | |||
| requested_interpreter_info = logged_exec(args) | |||
| except Exception as error: | |||
| logger.error("Error getting requested interpreter version: %s", error) | |||
| sys.exit(1) | |||
| raise | |||
There was a problem hiding this comment.
Use FadesError, as above (it's a good idea!!!)
fades/helpers.py
Outdated
| @@ -250,7 +250,7 @@ def check_pypi_exists(dependencies): | |||
| exists = _pypi_head_package(dependency) | |||
| except Exception as error: | |||
| logger.error("Error checking %s in PyPI: %r", dependency, error) | |||
| sys.exit(1) | |||
| raise | |||
fades/main.py
Outdated
| @@ -166,7 +166,7 @@ def go(): | |||
| getattr(sys, 'base_prefix', None)): | |||
| logger.error( | |||
| "fades is running from inside a virtualenv (%r), which is not supported", sys.prefix) | |||
| sys.exit(-1) | |||
| raise Exception | |||
fades/main.py
Outdated
| except Exception: | ||
| rc = 1 | ||
| logger.debug("CLEAN_UNUSED_VENVS must be an integer.") | ||
| except Exception.ValueError: |
There was a problem hiding this comment.
This is invalid, just use ValueError
fades/main.py
Outdated
| rc = 1 | ||
| logger.debug("CLEAN_UNUSED_VENVS must be an integer.") | ||
| except Exception.ValueError: | ||
| logger.debug("clean_unused_venvs must be an integer.") |
There was a problem hiding this comment.
Log in ERROR here, not DEBUG
fades/main.py
Outdated
| @@ -262,7 +260,7 @@ def go(): | |||
| "You can use '--no-precheck-availability' to avoid it.") | |||
| if not helpers.check_pypi_exists(indicated_deps): | |||
| logger.error("An indicated dependency doesn't exists. Exiting") | |||
| sys.exit(1) | |||
| raise Exception | |||
fades/main.py
Outdated
| @@ -304,7 +302,7 @@ def go(): | |||
| p = subprocess.Popen(cmd + args.child_options) | |||
| except FileNotFoundError: | |||
| logger.error("Command not found: %s", args.child_program) | |||
| sys.exit(1) | |||
| raise | |||
facundobatista
requested changes
May 1, 2018
fades/main.py
Outdated
| rc = 1 | ||
| logger.debug("CLEAN_UNUSED_VENVS must be an integer.") | ||
| except ValueError: | ||
| logger.error("clean_unused_venvs must be an integer.") | ||
| raise |
fades/main.py
Outdated
| finally: | ||
| sys.exit(rc) | ||
|
|
||
| usage_manager.clean_unused_venvs(max_days_to_keep) |
There was a problem hiding this comment.
After this operation, fades must quit (this was handled in the finally you removed)
testdev.bat
Outdated
| @@ -0,0 +1,11 @@ | |||
| @echo off | |||
|
|
|||
| rem Copyright 2014-2016 Facundo Batista, Nicolás Demarchi | |||
There was a problem hiding this comment.
2018 is enough here (this file is new)
facundobatista
approved these changes
May 1, 2018
|
Awesome! Thank you very much! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pass exceptions to the main module instead of sys.exiting.