-
Notifications
You must be signed in to change notification settings - Fork 84
android: create persistent SharedPreferences-based KV store #2319
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
Changes from all commits
fc0e26a
50a886a
7b3bc70
71dd680
2e2a33c
9db71b1
bb13d78
2133f87
f2db4bc
0ac4663
6e445fc
4605e11
4b6070f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| package io.envoyproxy.envoymobile.android | ||
|
|
||
| import android.content.SharedPreferences | ||
|
|
||
| import io.envoyproxy.envoymobile.KeyValueStore | ||
|
|
||
| /** | ||
| * Simple implementation of a `KeyValueStore` leveraging `SharedPreferences` for persistence. | ||
| */ | ||
| class SharedPreferencesStore(sharedPreferences: SharedPreferences) : KeyValueStore { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add some unit tests specific to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't today have a unit test setup that loads the Android platform APIs, because we don't today have a lot of Android-only or Android-specific code (by design). If/as we add more, we probably do want such a test suite, but this is mostly meant to be a trivial example implementation that guides others in using the API. That said, once we have a place for such tests to live, I agree we should cover this at the unit level for completeness' sake.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, thank you for the context. I would argue that we should have a test even for this trivial example since otherwise we cannot really tell whether it works or not the way we expect it to work - as in, I think that we are past the threshold when we can justify having an Android tests suite (if we have a need for Android code we have a need for Android tests). Could we create a GH issue for this if you do not want to do this as part of this PR? I would love us to start investing in testing more, otherwise it's going to become increasingly hard to continue to move forward without leaving broken stuff behind us.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Roboelectric supports this today, and we use it in quite a few tests: https://github.com/envoyproxy/envoy-mobile/search?q=robolectric |
||
| private val preferences = sharedPreferences | ||
| private val editor = sharedPreferences.edit() | ||
|
|
||
| override fun read(key: String): String? { | ||
| return preferences.getString(key, null) | ||
| } | ||
|
|
||
| override fun remove(key: String) { | ||
| editor.remove(key) | ||
| editor.apply() | ||
| } | ||
|
|
||
| override fun save(key: String, value: String) { | ||
| editor.putString(key, value) | ||
| editor.apply() | ||
| } | ||
| } | ||
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.
should we have some docs for this?
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.
Added docstring, and the interface is documented - would be great to generate docs from these.
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.
(Looks like @jpsim has made some great progress here!)