CLI: Replace ad-hoc failwith with structured Cli_error exception#93
Draft
CLI: Replace ad-hoc failwith with structured Cli_error exception#93
Conversation
febyeji
force-pushed
the
feature/cli-error-handling
branch
from
84c6b98 to
7410257
Compare
Collaborator
|
I think it's fine of both of those are setup errors |
febyeji
force-pushed
the
feature/cli-error-handling
branch
from
7410257 to
b8f71bc
Compare
- Add Error.ml with Cli_error exception, origin tags, and raise helpers - Replace bare failwith across all CLI modules - Restructure runner with nested try-with (setup errors vs model crashes) - Rename ParseError to SetupError in test_result
febyeji
force-pushed
the
feature/cli-error-handling
branch
from
b8f71bc to
35dfbd2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
CLI exception handling was ad-hoc — all errors used bare
failwith, sorunner.mlcouldn't distinguish parse errors from model crashes. This adds a structuredCli_errorexception with origin tags.Ordering
Design question
Parse errors and convert errors both map to
SetupErrorintest_result. The[origin]tag ([parser]vs[converter]) distinguishes them in output, buttest_resulthas a singleSetupErrorvariant for both. Should these be separate variants (e.g.SetupErrorvsConvertError)?cc @tperami