Repository: repobee/repobee

Issue:
Remove _sys_exit_on_expected_error context manager in cli
It just removes information, I have no idea of why I put it there in the first place.

Discussion:
codecov-io: # [Codecov](https://codecov.io/gh/repobee/repobee/pull/363?src=pr&el=h1) Report
> Merging [#363](https://codecov.io/gh/repobee/repobee/pull/363?src=pr&el=desc) into [master](https://codecov.io/gh/repobee/repobee/commit/47fe1df45f8b1805337090591fa372ae77c9fca2?src=pr&el=desc) will **decrease** coverage by `0.2%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/repobee/repobee/pull/363/graphs/tree.svg?width=650&token=5vOKgkphZe&height=150&src=pr)](https://codecov.io/gh/repobee/repobee/pull/363?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##           master    #363      +/-   ##
=========================================
- Coverage   87.81%   87.6%   -0.21%     
=========================================
  Files          21      21              
  Lines        1608    1581      -27     
  Branches      292     289       -3     
=========================================
- Hits         1412    1385      -27     
  Misses        172     172              
  Partials       24      24
```


| [Impacted Files](https://codecov.io/gh/repobee/repobee/pull/363?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/\_repobee/cli.py](https://codecov.io/gh/repobee/repobee/pull/363/diff?src=pr&el=tree#diff-c3JjL19yZXBvYmVlL2NsaS5weQ==) | `98.09% <100%> (-0.16%)` | :arrow_down: |
| [src/\_repobee/main.py](https://codecov.io/gh/repobee/repobee/pull/363/diff?src=pr&el=tree#diff-c3JjL19yZXBvYmVlL21haW4ucHk=) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/repobee/repobee/pull/363?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/repobee/repobee/pull/363?src=pr&el=footer). Last update [47fe1df...a392f06](https://codecov.io/gh/repobee/repobee/pull/363?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/src/_repobee/cli.py b/src/_repobee/cli.py
index fbb2143..5256e34 100644
--- a/src/_repobee/cli.py
+++ b/src/_repobee/cli.py
@@ -14,7 +14,6 @@ import pathlib
 import os
 import sys
 import re
-from contextlib import contextmanager
 from typing import List, Iterable, Optional, Tuple
 
 import logging
@@ -283,37 +282,28 @@ def dispatch_command(
     hook_results = {}
     if ext_command_names and args.subparser in ext_command_names:
         ext_cmd = ext_commands[ext_command_names.index(args.subparser)]
-        with _sys_exit_on_expected_error():
-            ext_cmd.callback(args, api)
+        ext_cmd.callback(args, api)
     elif args.subparser == SETUP_PARSER:
-        with _sys_exit_on_expected_error():
-            command.setup_student_repos(
-                args.master_repo_urls, args.students, api
-            )
+        command.setup_student_repos(args.master_repo_urls, args.students, api)
     elif args.subparser == UPDATE_PARSER:
-        with _sys_exit_on_expected_error():
-            command.update_student_repos(
-                args.master_repo_urls, args.students, api, issue=args.issue
-            )
+        command.update_student_repos(
+            args.master_repo_urls, args.students, api, issue=args.issue
+        )
     elif args.subparser == OPEN_ISSUE_PARSER:
-        with _sys_exit_on_expected_error():
-            command.open_issue(
-                args.issue, args.master_repo_names, args.students, api
-            )
+        command.open_issue(
+            args.issue, args.master_repo_names, args.students, api
+        )
     elif args.subparser == CLOSE_ISSUE_PARSER:
-        with _sys_exit_on_expected_error():
-            command.close_issue(
-                args.title_regex, args.master_repo_names, args.students, api
-            )
+        command.close_issue(
+            args.title_regex, args.master_repo_names, args.students, api
+        )
     elif args.subparser == MIGRATE_PARSER:
-        with _sys_exit_on_expected_error():
-            command.migrate_repos(args.master_repo_urls, api)
+        command.migrate_repos(args.master_repo_urls, api)
     elif args.subparser == CLONE_PARSER:
-        with _sys_exit_on_expected_error():
-            hook_results = command.clone_repos(
-                args.master_repo_names, args.students, api
-            )
-            LOGGER.info(formatters.format_hook_results_output(hook_results))
+        hook_results = command.clone_repos(
+            args.master_repo_names, args.students, api
+        )
+        LOGGER.info(formatters.format_hook_results_output(hook_results))
     elif args.subparser == VERIFY_PARSER:
         plug.manager.hook.get_api_class().verify_settings(
             args.user,
@@ -323,42 +313,35 @@ def dispatch_command(
             args.master_org_name,
         )
     elif args.subparser == LIST_ISSUES_PARSER:
-        with _sys_exit_on_expected_error():
-            hook_results = command.list_issues(
-                args.master_repo_names,
-                args.students,
-                api,
-                state=args.state,
-                title_regex=args.title_regex or "",
-                show_body=args.show_body,
-                author=args.author,
-            )
+        hook_results = command.list_issues(
+            args.master_repo_names,
+            args.students,
+            api,
+            state=args.state,
+            title_regex=args.title_regex or "",
+            show_body=args.show_body,
+            author=args.author,
+        )
     elif args.subparser == ASSIGN_REVIEWS_PARSER:
-        with _sys_exit_on_expected_error():
-            command.assign_peer_reviews(
-                args.master_repo_names,
-                args.students,
-                args.num_reviews,
-                args.issue,
-                api,
-            )
+        command.assign_peer_reviews(
+            args.master_repo_names,
+            args.students,
+            args.num_reviews,
+            args.issue,
+            api,
+        )
     elif args.subparser == PURGE_REVIEW_TEAMS_PARSER:
-        with _sys_exit_on_expected_error():
-            command.purge_review_teams(
-                args.master_repo_names, args.students, api
-            )
+        command.purge_review_teams(args.master_repo_names, args.students, api)
     elif args.subparser == SHOW_CONFIG_PARSER:
-        with _sys_exit_on_expected_error():
-            command.show_config()
+        command.show_config()
     elif args.subparser == CHECK_REVIEW_PROGRESS_PARSER:
-        with _sys_exit_on_expected_error():
-            command.check_peer_review_progress(
-                args.master_repo_names,
-                args.students,
-                args.title_regex,
-                args.num_reviews,
-                api,
-            )
+        command.check_peer_review_progress(
+            args.master_repo_names,
+            args.students,
+            args.title_regex,
+            args.num_reviews,
+            api,
+        )
     else:
         raise exception.ParseError(
             "Illegal value for subparser: {}. "
@@ -1006,30 +989,6 @@ def _add_traceback_arg(parser):
     )
 
 
-@contextmanager
-def _sys_exit_on_expected_error():
-    try:
-        yield
-    except exception.PushFailedError as exc:
-        LOGGER.error(
-            "There was an error pushing to {}. "
-            "Verify that your token has adequate access.".format(exc.url)
-        )
-        sys.exit(1)
-    except exception.CloneFailedError as exc:
-        LOGGER.error(
-            "There was an error cloning from {}. "
-            "Does the repo really exist?".format(exc.url)
-        )
-        sys.exit(1)
-    except exception.GitError:
-        LOGGER.error("Something went wrong with git. See the logs for info.")
-        sys.exit(1)
-    except exception.APIError as exc:
-        LOGGER.error("Exiting beacuse of {.__class__.__name__}".format(exc))
-        sys.exit(1)
-
-
 def _extract_groups(args: argparse.Namespace) -> List[str]:
     """Extract groups from args namespace.`
 
diff --git a/src/_repobee/main.py b/src/_repobee/main.py
index 6aec08f..946bb44 100644
--- a/src/_repobee/main.py
+++ b/src/_repobee/main.py
@@ -91,6 +91,7 @@ def main(sys_args: List[str]):
             LOGGER.exception("Critical exception")
         else:
             LOGGER.error("{.__class__.__name__}: {}".format(exc, str(exc)))
+        sys.exit(1)
 
 
 if __name__ == "__main__":


Test patch:
diff --git a/tests/unit_tests/test_cli.py b/tests/unit_tests/test_cli.py
index 3f21414..8b8724d 100644
--- a/tests/unit_tests/test_cli.py
+++ b/tests/unit_tests/test_cli.py
@@ -267,20 +267,6 @@ class TestDispatchCommand:
         that there are no crashes, does not validate any other behavior!"""
         cli.dispatch_command(parsed_args_all_subparsers, api_instance_mock)
 
-    def test_expected_exception_results_in_system_exit(
-        self,
-        parsed_args_all_subparsers,
-        api_instance_mock,
-        command_all_raise_mock,
-    ):
-        """Test that any of the expected exceptions results in SystemExit."""
-        if parsed_args_all_subparsers.subparser == cli.VERIFY_PARSER:
-            pytest.skip(
-                "verify-settings parser is exempt from the sysexit behavior"
-            )
-        with pytest.raises(SystemExit):
-            cli.dispatch_command(parsed_args_all_subparsers, api_instance_mock)
-
     def test_setup_student_repos_called_with_correct_args(
         self, command_mock, api_instance_mock
     ):
diff --git a/tests/unit_tests/test_main.py b/tests/unit_tests/test_main.py
index e750823..7bd11c5 100644
--- a/tests/unit_tests/test_main.py
+++ b/tests/unit_tests/test_main.py
@@ -97,23 +97,24 @@ def test_happy_path(
     )
 
 
-def test_does_not_raise_on_exception_in_parsing(
+def test_exit_status_1_on_exception_in_parsing(
     api_instance_mock, parse_args_mock, dispatch_command_mock, no_config_mock
 ):
-    """should just log, but not raise."""
     msg = "some nice error message"
     parse_args_mock.side_effect = raise_(Exception(msg))
     sys_args = ["just some made up arguments".split()]
 
-    main.main(sys_args)
+    with pytest.raises(SystemExit) as exc_info:
+        main.main(sys_args)
 
+    assert exc_info.value.code == 1
     parse_args_mock.assert_called_once_with(
         sys_args[1:], show_all_opts=False, ext_commands=DEFAULT_EXT_COMMANDS
     )
     assert not dispatch_command_mock.called
 
 
-def test_does_not_raise_on_exception_in_handling_parsed_args(
+def test_exit_status_1_on_exception_in_handling_parsed_args(
     api_instance_mock, parse_args_mock, dispatch_command_mock
 ):
     """should just log, but not raise."""
@@ -121,7 +122,13 @@ def test_does_not_raise_on_exception_in_handling_parsed_args(
     dispatch_command_mock.side_effect = raise_(Exception(msg))
     sys_args = ["just some made up arguments".split()]
 
-    main.main(sys_args)
+    with pytest.raises(SystemExit) as exc_info:
+        main.main(sys_args)
+
+    assert exc_info.value.code == 1
+    parse_args_mock.assert_called_once_with(
+        sys_args[1:], show_all_opts=False, ext_commands=DEFAULT_EXT_COMMANDS
+    )
 
 
 def test_plugins_args(
@@ -271,8 +278,10 @@ def test_logs_traceback_on_exception_in_dispatch_if_traceback(
     sys_args = ["repobee", *CLONE_ARGS, "--traceback"]
     dispatch_command_mock.side_effect = lambda *args, **kwargs: raise_()
 
-    main.main(sys_args)
+    with pytest.raises(SystemExit) as exc_info:
+        main.main(sys_args)
 
+    assert exc_info.value.code == 1
     assert logger_exception_mock.called
     parse_args_mock.assert_called_once_with(
         [*CLONE_ARGS, "--traceback"],
@@ -312,3 +321,19 @@ def test_show_all_opts_correctly_separated(
     parse_preparser_options_mock.assert_called_once_with(
         [cli.PRE_PARSER_SHOW_ALL_OPTS]
     )
+
+
+def test_non_zero_exit_status_on_exception(
+    parse_args_mock, parse_preparser_options_mock, no_config_mock
+):
+    def raise_(*args, **kwargs):
+        raise ValueError()
+
+    parse_args_mock.side_effect = raise_
+
+    sys_args = ["repobee", *CLONE_ARGS]
+
+    with pytest.raises(SystemExit) as exc_info:
+        main.main(sys_args)
+
+    assert exc_info.value.code == 1