Skip to content
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

iOS: add KeyValueStore protocol conformance to UserDefaults #2452

Merged
merged 4 commits into from
Aug 3, 2022

Conversation

jpsim
Copy link
Contributor

@jpsim jpsim commented Aug 2, 2022

Continued from #2441.

Description: Parallels Android implementation based on SharedPreferences.
Risk Level: Low
Testing: Application

Co-authored-by: Mike Schore mike.schore@gmail.com
Signed-off-by: JP Simard jp@jpsim.com

goaway and others added 2 commits August 2, 2022 14:51
Signed-off-by: Mike Schore <mike.schore@gmail.com>
In favor of adding `KeyValueStore` conformance to `UserDefaults`
directly.

Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
"filters/*.swift",
"grpc/*.swift",
"mocks/*.swift",
"stats/*.swift",
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I'd prefer we explicitly enumerate source files in build targets and avoid blobs. I feel it obscures information about what's being pulled in, and it has the potential to lead to surprises when files are moved around.

Upstream Envoy tends to follow the pattern of explicit enumeration of source files as well, even when that list is large.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I don't feel strongly but the previous approach was inconsistent with a mix of explicit files and globs.

As of 4d77493 everything is being explicitly set.

Copy link
Contributor

@goaway goaway left a comment

Choose a reason for hiding this comment

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

Thanks @jpsim, this is a nifty approach! Left one comment, but otherwise looks good.

As requested in code review.

Signed-off-by: JP Simard <jp@jpsim.com>
@jpsim jpsim requested a review from goaway August 3, 2022 14:29
@jpsim
Copy link
Contributor Author

jpsim commented Aug 3, 2022

/retest

@jpsim jpsim merged commit 044195d into main Aug 3, 2022
@jpsim jpsim deleted the jp/user-defaults branch August 3, 2022 16:13
RyanTheOptimist pushed a commit to RyanTheOptimist/envoy-mobile that referenced this pull request Aug 3, 2022
…yproxy#2452)

Continued from envoyproxy#2441.

Description: Parallels Android implementation based on SharedPreferences.
Risk Level: Low
Testing: Application

Co-authored-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants