mpenning / ciscoconfparse

Parse, Audit, Query, Build, and Modify Arista / Cisco / Juniper / Palo Alto / F5 configurations.
http://www.pennington.net/py/ciscoconfparse/
GNU General Public License v3.0
789 stars 219 forks source link

Change all logger.catch() decorators to use reraise=True #250

Closed daemonkeeper closed 1 year ago

daemonkeeper commented 1 year ago

Your code very much makes it impossible to use it as a library. You intercept exceptions without ever raising them again, and sink them into the wrapped loguru handler code for display. Unfortunately not everyone wants to see a fancy stack trace upon an exception on stderr (or use loguru on that matter), and instead might prefer to raise them to calling code, in order to properly handle it there (and/or use my logging system of choice to log it).

For example:

from ciscoconfparse import CiscoConfParse

try:
  CiscoConfParse("yolo")
except Exception as e:
  print("caught %s" % e)

never hits in my exception handler, because you catch and sink your ValueException into loguru:

In [6]: try:
   ...:     CiscoConfParse("yolo")
   ...: except Exception as e:
   ...:     print("caught %s" % e)
   ...:
2022-10-12 15:44:56.923 | ERROR    | ciscoconfparse.ciscoconfparse:__init__:485 - An error has been caught in function '__init__', process 'MainProcess' (75418), thread 'MainThread' (4303357312):
Traceback (most recent call last):

  File "/opt/homebrew/bin/ipython", line 8, in <module>
    sys.exit(start_ipython())
    │   │    └ <function start_ipython at 0x1029a1750>
    │   └ <built-in function exit>
    └ <module 'sys' (built-in)>
  File "/opt/homebrew/Cellar/ipython/8.4.0/libexec/lib/python3.10/site-packages/IPython/__init__.py", line 124, in start_ipython
    return launch_new_instance(argv=argv, **kwargs)
           │                        │       └ {}
           │                        └ None
           └ <bound method Application.launch_instance of <class 'IPython.terminal.ipapp.TerminalIPythonApp'>>
  File "/opt/homebrew/Cellar/ipython/8.4.0/libexec/lib/python3.10/site-packages/traitlets/config/application.py", line 972, in launch_instance
    app.start()
    │   └ <function TerminalIPythonApp.start at 0x1029a0d30>
    └ <IPython.terminal.ipapp.TerminalIPythonApp object at 0x100ad2b30>
  File "/opt/homebrew/Cellar/ipython/8.4.0/libexec/lib/python3.10/site-packages/IPython/terminal/ipapp.py", line 318, in start
    self.shell.mainloop()
    │    └ <traitlets.traitlets.Instance object at 0x1029996c0>
    └ <IPython.terminal.ipapp.TerminalIPythonApp object at 0x100ad2b30>
  File "/opt/homebrew/Cellar/ipython/8.4.0/libexec/lib/python3.10/site-packages/IPython/terminal/interactiveshell.py", line 680, in mainloop
    self.interact()
    │    └ <function TerminalInteractiveShell.interact at 0x102849c60>
    └ <IPython.terminal.interactiveshell.TerminalInteractiveShell object at 0x10299a050>
  File "/opt/homebrew/Cellar/ipython/8.4.0/libexec/lib/python3.10/site-packages/IPython/terminal/interactiveshell.py", line 673, in interact
    self.run_cell(code, store_history=True)
    │    │        └ 'try:\n    CiscoConfParse("yolo")\nexcept Exception as e:\n    print("caught %s" % e)\n    '
    │    └ <function InteractiveShell.run_cell at 0x102023be0>
    └ <IPython.terminal.interactiveshell.TerminalInteractiveShell object at 0x10299a050>
  File "/opt/homebrew/Cellar/ipython/8.4.0/libexec/lib/python3.10/site-packages/IPython/core/interactiveshell.py", line 2881, in run_cell
    result = self._run_cell(
             │    └ <function InteractiveShell._run_cell at 0x102023c70>
             └ <IPython.terminal.interactiveshell.TerminalInteractiveShell object at 0x10299a050>
  File "/opt/homebrew/Cellar/ipython/8.4.0/libexec/lib/python3.10/site-packages/IPython/core/interactiveshell.py", line 2936, in _run_cell
    return runner(coro)
           │      └ <coroutine object InteractiveShell.run_cell_async at 0x1019e1150>
           └ <function _pseudo_sync_runner at 0x10201b760>
  File "/opt/homebrew/Cellar/ipython/8.4.0/libexec/lib/python3.10/site-packages/IPython/core/async_helpers.py", line 129, in _pseudo_sync_runner
    coro.send(None)
    │    └ <method 'send' of 'coroutine' objects>
    └ <coroutine object InteractiveShell.run_cell_async at 0x1019e1150>
  File "/opt/homebrew/Cellar/ipython/8.4.0/libexec/lib/python3.10/site-packages/IPython/core/interactiveshell.py", line 3135, in run_cell_async
    has_raised = await self.run_ast_nodes(code_ast.body, cell_name,
                       │    │             │        │     └ '<ipython-input-6-3164f275a5cc>'
                       │    │             │        └ [<ast.Try object at 0x1059ba770>]
                       │    │             └ <ast.Module object at 0x1059b93f0>
                       │    └ <function InteractiveShell.run_ast_nodes at 0x102028040>
                       └ <IPython.terminal.interactiveshell.TerminalInteractiveShell object at 0x10299a050>
  File "/opt/homebrew/Cellar/ipython/8.4.0/libexec/lib/python3.10/site-packages/IPython/core/interactiveshell.py", line 3338, in run_ast_nodes
    if await self.run_code(code, result, async_=asy):
             │    │        │     │              └ False
             │    │        │     └ <ExecutionResult object at 103238df0, execution_count=6 error_before_exec=None error_in_exec=None info=<ExecutionInfo object ...
             │    │        └ <code object <cell line: 1> at 0x103260870, file "<ipython-input-6-3164f275a5cc>", line 1>
             │    └ <function InteractiveShell.run_code at 0x1020280d0>
             └ <IPython.terminal.interactiveshell.TerminalInteractiveShell object at 0x10299a050>
  File "/opt/homebrew/Cellar/ipython/8.4.0/libexec/lib/python3.10/site-packages/IPython/core/interactiveshell.py", line 3398, in run_code
    exec(code_obj, self.user_global_ns, self.user_ns)
         │         │    │               │    └ {'__name__': '__main__', '__doc__': 'Automatically created module for IPython interactive environment', '__package__': None, ...
         │         │    │               └ <IPython.terminal.interactiveshell.TerminalInteractiveShell object at 0x10299a050>
         │         │    └ <property object at 0x102005f80>
         │         └ <IPython.terminal.interactiveshell.TerminalInteractiveShell object at 0x10299a050>
         └ <code object <cell line: 1> at 0x103260870, file "<ipython-input-6-3164f275a5cc>", line 1>
  File "<ipython-input-6-3164f275a5cc>", line 2, in <cell line: 1>
    CiscoConfParse("yolo")
    └ <class 'ciscoconfparse.ciscoconfparse.CiscoConfParse'>
> File "/opt/homebrew/lib/python3.10/site-packages/ciscoconfparse/ciscoconfparse.py", line 485, in __init__
    config = self.get_config_lines(config=config, logger=logger)
             │    │                       │              └ <loguru.logger handlers=[(id=1, level=10, sink=<stdout>)]>
             │    │                       └ 'yolo'
             │    └ <function CiscoConfParse.get_config_lines at 0x10320caf0>
             └ <CiscoConfParse: None lines / syntax: ios / comment delimiter: '!' / factory: False / encoding: 'UTF-8'>
  File "/opt/homebrew/lib/python3.10/site-packages/ciscoconfparse/ciscoconfparse.py", line 602, in get_config_lines
    raise ValueError("config='%s' is an unexpected type()" % config)
                                                             └ 'yolo'

ValueError: config='yolo' is an unexpected type()
---------------------------------------------------------------------------

Since you unambiguously always call configure_loguru() e.g; in ciscoconfparse.py) you may want to give the consumers of your classes a way to intercept and configure the behavior.

I am able to disable the fancy stack trace output by calling ccp_logger_control(action="disable"), but that won't raise them back to my code because of the missing 'reraise' when initializing the loguru logger. Moreover, I'd argue re-raising exceptions should be the default anyway.

As a strech goal you may want to consider letting users set their own (non-loguru) handlers to catch your log output and properly sink it wherever my system wants.

vincentbernat commented 1 year ago

Moreover, the exceptions are fatal as they call sys.exit(1).

mpenning commented 1 year ago

Moreover, the exceptions are fatal as they call sys.exit(1).

I want unhandled exceptions to be fatal.

daemonkeeper commented 1 year ago

Thanks for the quick fix. I've tested your change and now I do get the exceptions raised back into my code, allowing me to handle them properly there. Thanks.

mpenning commented 1 year ago

NOTE I also added a unit test for this case at git commit hash 07ff028d14fcf603712c8556bf2f324466d08782... see testParse_invalid_config() in test_CiscoConfParse.py

mpenning commented 1 year ago

Documenting a complete try / except / finally example under ciscoconfparse version 1.6.44...

In both reraise cases, logger.catch() sends loguru error output to the terminal session. The only difference is whether the except block runs.

# filename is 'try_test.py'
from loguru import logger

@logger.catch(reraise=True)
def simulate_ccp_function():
    raise NotImplementedError()

def user_function():
    try:
        print("'try' Before calling simulate_ccp_function()")
        simulate_ccp_function()
        print("    'try' After calling simulate_ccp_function()")
    except:
        print("    'except' was executed")
    finally:
        print("    'finally' was executed")

if __name__=="__main__":
    user_function()

When that script runs, we get the following output... image

mpenning commented 1 year ago

Also see the question I asked about try / except usage in delgan/loguru...

https://github.com/Delgan/loguru/issues/723