Should we replace sys.exit() with exception?

[fornwall]

We currently have a sys.exit(1) call at https://github.com/meeshkan/meeshkan/blob/master/meeshkan/server/server/callbacks.py#L42.

As @ksaaskil mentioned in a review comment, perhaps we should replace that by raising an exception, and catching it higher up. What do you think? My initial thoughts are that it's ok to call sys.exit directly for command-line programs, but not for any code that might be used as a library function. But I'm easily persuaded here!

[fornwall]

Ping @aby2s and @mikesol as well.

[ksaaskil]

I think there's nothing inside CallbackManager saying it's meant to be used either in command-line scripts or as Python library, so that was kinda my logic to propose throwing an exception 🙂

[aby2s]

An exception could be much better. We don't even need to catch it because Python script will stop by itself producing a stack trace with a meaningful error description.

Actually, the meeshkan server can run embedded by anyone:

    server = MockServer(port=port)
    server.run()

It is not a documented feature but it exists and I don't see any reasons to lose it if doesn't cost us anything.

[fornwall]

We don't even need to catch it because Python script will stop by itself producing a stack trace with a meaningful error description.

If we apply something like this

diff --git a/meeshkan/serve/mock/callbacks.py b/meeshkan/serve/mock/callbacks.py
index 6c68c41..cd9a8de 100644
--- a/meeshkan/serve/mock/callbacks.py
+++ b/meeshkan/serve/mock/callbacks.py
@@ -38,8 +38,7 @@ class CallbackManager:

     def load(self, path):
         if not os.path.exists(path):
-            logger.fatal("Callback configuration path doesn't exist: %s", path)
-            sys.exit(1)
+            raise FileNotFoundError("Callback configuration path doesn't exist: %s", path)

         for f in glob.glob(os.path.join(path, '*.py')):
             module_name = 'callbacks.{}'.format(os.path.splitext(os.path.basename(f))[0])

and we request a non-existing directory, we get this:

$ meeshkan mock --callback-dir asdfasdf
2020-03-10 11:05:24,210 - meeshkan.serve.admin.runner - INFO - Starting admin endpont on http://localhost:8888/admin
Traceback (most recent call last):
  File "/Users/fornwall/src/meeshkan/meeshkan/.venv/bin/meeshkan", line 11, in <module>
    load_entry_point('meeshkan==0.2.16', 'console_scripts', 'meeshkan')()
  File "/Users/fornwall/src/meeshkan/meeshkan/.venv/lib/python3.8/site-packages/click/core.py", line 764, in __call__
    return self.main(*args, **kwargs)
  File "/Users/fornwall/src/meeshkan/meeshkan/.venv/lib/python3.8/site-packages/click/core.py", line 717, in main
    rv = self.invoke(ctx)
  File "/Users/fornwall/src/meeshkan/meeshkan/.venv/lib/python3.8/site-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/fornwall/src/meeshkan/meeshkan/.venv/lib/python3.8/site-packages/click/core.py", line 1114, in invoke
    return Command.invoke(self, ctx)
  File "/Users/fornwall/src/meeshkan/meeshkan/.venv/lib/python3.8/site-packages/click/core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/fornwall/src/meeshkan/meeshkan/.venv/lib/python3.8/site-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/Users/fornwall/src/meeshkan/meeshkan/.venv/lib/python3.8/site-packages/click/decorators.py", line 17, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/Users/fornwall/src/meeshkan/meeshkan/.venv/lib/python3.8/site-packages/meeshkan-0.2.16-py3.8.egg/meeshkan/serve/commands.py", line 49, in mock
    ctx.forward(start_mock)
  File "/Users/fornwall/src/meeshkan/meeshkan/.venv/lib/python3.8/site-packages/click/core.py", line 573, in forward
    return self.invoke(cmd, **kwargs)
  File "/Users/fornwall/src/meeshkan/meeshkan/.venv/lib/python3.8/site-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/Users/fornwall/src/meeshkan/meeshkan/.venv/lib/python3.8/site-packages/meeshkan-0.2.16-py3.8.egg/meeshkan/serve/commands.py", line 67, in start_mock
    server.run()
  File "/Users/fornwall/src/meeshkan/meeshkan/.venv/lib/python3.8/site-packages/meeshkan-0.2.16-py3.8.egg/meeshkan/serve/mock/server.py", line 44, in run
    app = make_mocking_app(self._callback_dir, self._specs_dir, self._routing)
  File "/Users/fornwall/src/meeshkan/meeshkan/.venv/lib/python3.8/site-packages/meeshkan-0.2.16-py3.8.egg/meeshkan/serve/mock/server.py", line 26, in make_mocking_app
    callback_manager.load(callback_dir)
  File "/Users/fornwall/src/meeshkan/meeshkan/.venv/lib/python3.8/site-packages/meeshkan-0.2.16-py3.8.egg/meeshkan/serve/mock/callbacks.py", line 41, in load
    raise FileNotFoundError("Callback configuration path doesn't exist: %s", path)
FileNotFoundError: [Errno Callback configuration path doesn't exist: %s] asdfasdf

That's not very friendly - I don't think command-line tools should show stack traces by default for usage errors (while it could show it for internal errors). What about having a top-level catch clause, and only show stack traces if a --verbose flag has been specified`?

[fornwall]

See #108.

[aby2s]

I think it is fine for a developer tool. Developers are hard to scare with a stack trace.