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.

Sign up for GitHub

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

Jump to bottom

cc: add support for registering a platform KV store #2430

Merged
merged 6 commits into from
Aug 3, 2022
Merged

cc: add support for registering a platform KV store #2430

merged 6 commits into from
Aug 3, 2022

Conversation

goaway
Copy link
Contributor

@goaway goaway commented Jul 19, 2022

Signed-off-by: Mike Schore mike.schore@gmail.com

@goaway
cc: add support for registering a platform KV store
a2f91e8 
Signed-off-by: Mike Schore <mike.schore@gmail.com>
@goaway
format
55eb105 
Signed-off-by: Mike Schore <mike.schore@gmail.com>
@goaway goaway marked this pull request as ready for review July 28, 2022 21:50
Augustyniak
Augustyniak reviewed Jul 28, 2022
View reviewed changes
Copy link
Contributor

@Augustyniak Augustyniak left a comment

Choose a reason for hiding this comment

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

Did the first pass. Have a question related to the use of enable_shared_from_this

library/cc/key_value_store.h Outdated Show resolved Hide resolved
library/cc/key_value_store.h Show resolved Hide resolved
library/cc/key_value_store.cc Show resolved Hide resolved
library/cc/key_value_store.cc Show resolved Hide resolved
@Augustyniak
Copy link
Contributor

Augustyniak commented Jul 28, 2022 •
edited

  1. at the high level I do not see any issues with this PR other than a lack of an integration test. We talked offline and you mentioned that you are planning to address this issue in a follow up PR - please either add a test in this PR or create a GH issue for adding an integration test.
  2. Let's also make sure to update version_history file.

@goaway
Merge branch 'main' into ms/cc-kv-store
60b0289 
Signed-off-by: Mike Schore <mike.schore@gmail.com>
@goaway
PR comments
162e310 
Signed-off-by: Mike Schore <mike.schore@gmail.com>
@goaway
Copy link
Contributor Author

goaway commented Aug 2, 2022

#2451

@goaway
update version history
2bc2ea9 
Signed-off-by: Mike Schore <mike.schore@gmail.com>
@goaway goaway requested a review from Augustyniak August 2, 2022 18:58
jpsim
jpsim reviewed Aug 2, 2022
View reviewed changes
library/cc/key_value_store.h Outdated Show resolved Hide resolved
jpsim
jpsim reviewed Aug 2, 2022
View reviewed changes
docs/root/intro/version_history.rst
@@ -32,7 +32,7 @@ Bugfixes:

Features:

- Android & iOS: add support for registering a platform KV store (:issue: `#2134 <2134>`) (:issue: `#2335 <2335>`)
- Android, iOS, & C++: add support for registering a platform KV store (:issue: `#2134 <2134>`, :issue: `#2335 <2335>`, :issue: `#2430 <2430>`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we get into a habit of referring to the various APIs by language instead of a mix of platform and language?

Android, iOS, & C++ -> Kotlin, Swift & C++

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No strong feelings. This labeling is purely historical, and we've just stuck with it to be consistent. To your point, the library itself is organized by language. The only point I can think of in favor of the platform names is that it perhaps highlights that the library is intended for use in these environments to the unfamiliar user. But the README can do that, too.

I'm good either way.

Copy link
Contributor

@jpsim jpsim Aug 3, 2022 •
edited

Choose a reason for hiding this comment

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

Not really related to this PR, so I'm happy to take the discussion async.

jpsim
jpsim previously approved these changes Aug 2, 2022
View reviewed changes
@goaway
typo
76dc182 
Signed-off-by: Mike Schore <mike.schore@gmail.com>
@goaway goaway merged commit 308f15c into main Aug 3, 2022
@goaway goaway deleted the ms/cc-kv-store branch August 3, 2022 17:47
@goaway
Copy link
Contributor Author

goaway commented Aug 3, 2022

Thanks @Augustyniak and @jpsim for the feedback!

@goaway goaway mentioned this pull request Aug 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpsim jpsim jpsim approved these changes

@Augustyniak Augustyniak Awaiting requested review from Augustyniak

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@goaway @Augustyniak @jpsim