-
Notifications
You must be signed in to change notification settings - Fork 393
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
Correctly handle error scenario and propagate error message up #252
Conversation
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.
Thanks @tovbinm for updating this! LGTM
val distString = (json \ rawFeatureDistributionsEntryName).extract[String] | ||
FeatureDistribution.fromJson(distString).map(d => RawFeatureFilterResults(rawFeatureDistributions = d)) | ||
} else { | ||
Success(RawFeatureFilterResults()) |
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.
Do we have a test for this issue?
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.
We don't have a test for loading an old model with rawFeatureDistributions
defined
I'll save a model with previous TMOG version in src/test/resources/
and create a test
Codecov Report
@@ Coverage Diff @@
## master #252 +/- ##
==========================================
- Coverage 86.53% 82.59% -3.95%
==========================================
Files 314 314
Lines 10297 10294 -3
Branches 331 554 +223
==========================================
- Hits 8911 8502 -409
- Misses 1386 1792 +406
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #252 +/- ##
==========================================
+ Coverage 86.53% 86.54% +<.01%
==========================================
Files 314 314
Lines 10297 10294 -3
Branches 331 570 +239
==========================================
- Hits 8911 8909 -2
+ Misses 1386 1385 -1
Continue to review full report at Codecov.
|
Thanks for the contribution! It looks like @tovbinm is an internal user so signing the CLA is not required. However, we need to confirm this. |
Thanks for the contribution! Unfortunately we can't verify the commit author(s): Matthew <m***@s***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, refresh the status of this Pull Request. |
Related issues
Error message was not propagated to the user following up on #237 change
Describe the proposed solution
Propagate the error and return Try correctly
Describe alternatives you've considered
NA