v2 Java changes for consistency with other SDKs #59
Conversation
| /** | ||
| * An error indicating an abnormal result from evaluating a feature | ||
| */ | ||
| public class EvaluationError extends Exception { |
There was a problem hiding this comment.
A new exception to help us distinguish precondition / well-formedness failures during evaluation from null evaluation results, which should result in the offVariation being served.
There was a problem hiding this comment.
This should not be public- we don't throw it from any public method.
Changing the name to EvaluationException is more consistent with Java naming conventions, unless we ARE going to throw this from a public method (probly not a good idea), then we should name it LDEvaluationException or similar.
| prereqOk = false; | ||
| } | ||
| } catch (EvaluationError err) { | ||
| prereqOk = false; |
There was a problem hiding this comment.
I think we should log the exception here- at debug or warn.
|
@drichelson think I've covered all the PR feedback. Can you circle back when you have a chance? |
| /** | ||
| * An error indicating an abnormal result from evaluating a feature | ||
| */ | ||
| class EvaluationError extends Exception { |
There was a problem hiding this comment.
My original comment was 2 parts, the 2nd part was not addressed/discussed:
Changing the name to EvaluationException is more consistent with Java naming conventions.
|
Should be all set now. |
|
👍 |
treat unsupported operators as a non-match, don't throw NPE
I'll walk through the changes in comments below.