-
Notifications
You must be signed in to change notification settings - Fork 7
Description
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 mockingsys.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 newnode_cli/exceptions.pymodule. Examples might includeConfigurationError,DockerOperationError,NodeStateError,NetworkError, etc. - Replace
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. - Centralize Exception Handling: Enhance the main
try...exceptblock innode_cli/main.py.- Add specific
exceptclauses 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 inmain.pyto 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.
- Add specific
- Update Unit Tests: Modify existing tests that currently assert
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
- 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.