Is your feature request related to a problem? Please describe.
Currently, the node-cli codebase extensively uses the error_exit() function (imported from node_cli.utils.helper) to handle error conditions and terminate the program execution with specific exit codes.
This pattern is found across various modules (e.g., core, operations, configs).
While error_exit() provides a way to exit with a specific code, this approach has several drawbacks:
- Decentralized Control Flow: Errors cause immediate termination deep within function calls, making it harder to understand and manage the program's exit points.
- Limited Error Information: Exceptions can carry more context and structured information about the error than simple exit codes and messages.
- Reduced Flexibility: It prevents higher-level code from potentially catching specific errors to perform cleanup, logging, or alternative actions before exiting.
- Testing Complexity: Testing functions that call
error_exit() often requires mocking sys.exit, which can be less straightforward than asserting that specific exceptions are raised.
- Deviation from Pythonic Practices: Standard Python error handling relies on raising and catching exceptions.
This issue was highlighted during a recent Pull Request review (PR #839) with the comment: "It is cleaner to handle base Exception class in one place. It is already handled in main.py. Suggest to raise custom exception type instead and run error_exit in main.py with specific error index."
Describe the solution you'd like
We propose refactoring the codebase to adopt a more standard exception-based error handling mechanism:
- Define Custom Exception Classes: Create a set of custom exception classes, potentially inheriting from a common base exception (e.g.,
NodeCliError). These could reside in a new node_cli/exceptions.py module. Examples might include ConfigurationError, DockerOperationError, NodeStateError, NetworkError, etc.
- Replace
error_exit() with raise: Identify instances where error_exit() is called due to a specific error condition. Replace these calls with raise CustomException(...), passing relevant context if possible. Allow standard exceptions (like PermissionError, FileNotFoundError) to propagate where appropriate instead of catching them and calling error_exit.
- Centralize Exception Handling: Enhance the main
try...except block in node_cli/main.py.
- Add specific
except clauses for the newly defined custom exceptions (or their base class) and potentially common standard exceptions if specific exit codes are desired for them.
- Inside these handlers, map the caught exception type to the appropriate exit code (from
node_cli.utils.exit_codes).
- Call the
error_exit() function only from within these central handlers in main.py to perform the final exit operation with the correct code and message.
- Maintain the generic
except Exception: handler to catch unexpected errors, ensuring it logs sufficient details (like the traceback) before exiting.
- Update Unit Tests: Modify existing tests that currently assert
SystemExit based on error_exit calls (e.g., using pytest.raises(SystemExit) and checking excinfo.value.code). These tests must be updated to expect the new custom exceptions or relevant standard exceptions (like PermissionError, FileNotFoundError, etc.) where appropriate (e.g., using pytest.raises(SpecificException)).
This approach will centralize program termination logic, make error handling more explicit and flexible, and improve code maintainability and testability.
Describe alternatives you've considered
- Maintaining Status Quo: Continue using
error_exit() extensively. This perpetuates the issues mentioned above and requires specific handling in tests.
- Returning Error Codes/Objects: Functions could return status indicators or objects instead of raising exceptions. This is generally less Pythonic for handling exceptional situations and can lead to verbose checking of return values throughout the code.
The proposed custom exception approach aligns best with standard practices and offers the most benefits for code clarity and robustness.
Additional context
This is a significant but valuable refactoring effort that will improve the overall quality and maintainability of the node-cli codebase.
It standardizes how errors are signaled and handled, making the application easier to debug and extend in the future.
Is your feature request related to a problem? Please describe.
Currently, the
node-clicodebase extensively uses theerror_exit()function (imported fromnode_cli.utils.helper) to handle error conditions and terminate the program execution with specific exit codes.This pattern is found across various modules (e.g.,
core,operations,configs).While
error_exit()provides a way to exit with a specific code, this approach has several drawbacks:error_exit()often requires mockingsys.exit, which can be less straightforward than asserting that specific exceptions are raised.This issue was highlighted during a recent Pull Request review (PR #839) with the comment: "It is cleaner to handle base Exception class in one place. It is already handled in main.py. Suggest to raise custom exception type instead and run error_exit in main.py with specific error index."
Describe the solution you'd like
We propose refactoring the codebase to adopt a more standard exception-based error handling mechanism:
NodeCliError). These could reside in a newnode_cli/exceptions.pymodule. Examples might includeConfigurationError,DockerOperationError,NodeStateError,NetworkError, etc.error_exit()withraise: Identify instances whereerror_exit()is called due to a specific error condition. Replace these calls withraise CustomException(...), passing relevant context if possible. Allow standard exceptions (likePermissionError,FileNotFoundError) to propagate where appropriate instead of catching them and callingerror_exit.try...exceptblock innode_cli/main.py.exceptclauses for the newly defined custom exceptions (or their base class) and potentially common standard exceptions if specific exit codes are desired for them.node_cli.utils.exit_codes).error_exit()function only from within these central handlers inmain.pyto perform the final exit operation with the correct code and message.except Exception:handler to catch unexpected errors, ensuring it logs sufficient details (like the traceback) before exiting.SystemExitbased onerror_exitcalls (e.g., usingpytest.raises(SystemExit)and checkingexcinfo.value.code). These tests must be updated to expect the new custom exceptions or relevant standard exceptions (likePermissionError,FileNotFoundError, etc.) where appropriate (e.g., usingpytest.raises(SpecificException)).This approach will centralize program termination logic, make error handling more explicit and flexible, and improve code maintainability and testability.
Describe alternatives you've considered
error_exit()extensively. This perpetuates the issues mentioned above and requires specific handling in tests.The proposed custom exception approach aligns best with standard practices and offers the most benefits for code clarity and robustness.
Additional context
This is a significant but valuable refactoring effort that will improve the overall quality and maintainability of the
node-clicodebase.It standardizes how errors are signaled and handled, making the application easier to debug and extend in the future.