Experiments public contract#243
Merged
wzieba merged 23 commits intoexperimentation_refactorfrom Sep 5, 2024
Merged
Conversation
Because this is what ExPlat practically is - a repository for `Variation`. Extracting an interface makes it easier for consumers to introduce a test double for this
…method This was it's more readable IMO and is similar to how other systems work (especially in CMD) - the operation is requested and can perform the action or not depending on circumstances. But it also has a `-f` parameter.
So we can be aware of any changes to the public contract.
I don't think `interface` is very useful here. Value class is just fine and makes the API cleaner
We want SDK to provide easy API, and not bother clients with necessary requests (like calling `refresh` manually on every initialization)
wzieba
commented
Aug 29, 2024
| public class ExPlat internal constructor( | ||
| private val platform: String, | ||
| private val experiments: Set<Experiment>, | ||
| experiments: Set<Experiment>, |
Member
Author
There was a problem hiding this comment.
I wonder: is there a value in providing experiments in the constructor and then checking for its existence in getVariation? I don't see good benefits 🤔
ParaskP7
reviewed
Aug 29, 2024
Contributor
ParaskP7
left a comment
ParaskP7
left a comment
There was a problem hiding this comment.
And with that final review, I think I am done with all the review @wzieba , kudos on this work, it will help our product teams setup and start using ExPlat without much overhead, and mostly, without FluxC! 🎉 🥇 🚀
Let's start resolving the comments one-by-one and merge everything to trunk when ready, just let me know how to help you from here! 💯
| plugins: [$CI_TOOLKIT] | ||
| command: | | ||
| cp gradle.properties-example gradle.properties | ||
| ./gradlew :experimentation:apiCheck |
Contributor
There was a problem hiding this comment.
Praise (❤️): Awesome, love it, thanks for adding the binary-compatibility-validator plugin into this repo, let's start using this more and more within this project and other repos as well! 🥇
| @@ -0,0 +1,63 @@ | |||
| # Experimentation | |||
Contributor
There was a problem hiding this comment.
Praise (❤️): Thanks for creating a README.md to help with the API usage!
experimentation/src/main/java/com/automattic/android/experimentation/ExPlat.kt
Outdated
Show resolved
Hide resolved
| * | ||
| * Should be a Singleton, as it keeps track of the "active" [Variation]s. See [getVariation] for more details. | ||
| */ | ||
| public interface VariationsRepository { |
wzieba
commented
Aug 30, 2024
experimentation/src/main/java/com/automattic/android/experimentation/VariationsRepository.kt
Outdated
Show resolved
Hide resolved
This was referenced Aug 30, 2024
Base automatically changed from
improve_threading_and_test_suite
to
experimentation_refactor
August 30, 2024 12:14
This commit adds a new class: `CacheDto` that saves previously saved data with addition to `anonymousId`. This additional field will be used for cache validation.
The `initialize` method now takes a mandatory `anonymousId` parameter and is responsible for setting the anonymous identifier and triggering the initial assignment refresh and cache validation if `anonymousId` is different
I came to a conclusion, that `refresh` is not needed for the client: SDK should on its own manage the state of the cache and not delegate additional responsibility to SDK consumer. The cache is refreshed whenever needed during initialization
As SDK doesn't offer refreshing anymore, we don't need to use internal-only `RefreshStrategy`. Refactor seems successful as unit tests are still passing without any changes
If ExPlat was not initialized, log or throw an exception for quicker debugging
It doesn't have to be exposed to SDK consumers
…mandatory Make anon id mandatory. Simplify API, remove `refresh`
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.
Description
This PR introduces improvements to the public contract:
VariationsRepository, so the clients can easily double this in tests + KDoc commentsrefreshAPI (no need for two methods)README.mdwith an explanation about how to use the SDKTesting
The only behavior change has been covered with automated tests. It's still possible to smoke test using instructions from #241 , if necessary.