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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add api for loading plans of all types #80
feat: add api for loading plans of all types #80
Changes from 3 commits
2dfee3b
6fa011e
cceac79
c29dbbb
9aae897
f8c383a
01ac34b
fe0ddad
28c9e39
b0a956f
77ecaf3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the load and save methods return
StatusOr
then I, as a user, would not expect to get an exception. I think we still rely on exceptions in some places (maybe this isn't true?) If so, we should probably wrap these methods with a try/catch and wrap the exception in an invalid status.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed most of the cases where we use SUBSTRAIT_FAIL. There are about 5 remaining and most of those are for nearly impossible cases. I'd prefer to finish removing them over wrapping this code in an exception. If it turns out we're getting exceptions from other libraries then we will be forced to add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(And the fuzz testing work will help illuminate if we need to wrap the code.)