Skip to content

Conversation

@fayyazarshad
Copy link
Contributor

@fayyazarshad fayyazarshad commented Dec 20, 2019

Summary

Updated the OptimizelyConfig implementation to always return cached object. OptimizelyConfig object will only be updated only when new ProjectConfig is received.

Test Plan

Added Unit tests

@fayyazarshad fayyazarshad self-assigned this Dec 20, 2019
@coveralls
Copy link

coveralls commented Dec 20, 2019

Pull Request Test Coverage Report for Build 1332

  • 7 of 24 (29.17%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 89.278%

Changes Missing Coverage Covered Lines Changed/Added Lines %
core-api/src/main/java/com/optimizely/ab/Optimizely.java 4 12 33.33%
core-api/src/main/java/com/optimizely/ab/optimizelyconfig/OptimizelyConfig.java 0 9 0.0%
Totals Coverage Status
Change from base Build 1326: -0.2%
Covered Lines: 26229
Relevant Lines: 29379

💛 - Coveralls

@fayyazarshad fayyazarshad force-pushed the fayyaz/integrate-optimizely-config branch from e648907 to 4e9aba8 Compare December 30, 2019 13:45
@fayyazarshad fayyazarshad force-pushed the fayyaz/integrate-optimizely-config branch from 83e89f6 to ce9d305 Compare January 3, 2020 07:39
@zashraf1985 zashraf1985 changed the title Added Caching in Optimizely Config feat: Added caching for OptimizelyConfig object Jan 3, 2020
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@mikecdavis mikecdavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one request. Let's add the OptimizelyConfigManager to the constructor as a @Nullable parameter and handle the default from the builder. I prefer to keep the constructor devoid of business logic.

@zashraf1985 zashraf1985 merged commit 2e63da9 into master Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants