Conversation
Co-authored-by: Tom Andersen <tom-andersen@users.noreply.github.com>
…pp-sdk into mila/MultiDB
wu-hui
left a comment
There was a problem hiding this comment.
Close to LGTM!
I do want to see less usage of const char* though, we should try to avoid raw pointers, especially when database id is not expensive to copy around.
firestore/integration_test_internal/src/util/locate_emulator.cc
Outdated
Show resolved
Hide resolved
| * the given database ID. | ||
| */ | ||
| static Firestore* GetInstance(::firebase::App* app, | ||
| const char* db_name, |
There was a problem hiding this comment.
I dont have the API proposal at hand, but what is the reason to use const char* here instead of a const std::string& ?
There was a problem hiding this comment.
In the original API proposal , it was const std::string&, and later requested to be changed to const char*. I am not sure why.
There was a problem hiding this comment.
OK, at least we should find out why. I suppose there is a reason they asked for this.
❌ Integration test FAILEDRequested by @milaGGL on commit 35f554b
Add flaky tests to go/fpl-cpp-flake-tracker |
Add multiDB support.
Ported from:
TODO:
Firestore::GetInstancepublic interface to accept dynamic database namestd::pair<App*, std::string>as key instead ofApp*, to identify DBs in the same app while creating and deleting firestore instances.TestFirestoreWithDatabaseIdandIsUsingFirestoreEmulatortest helper method- New
GetInstancemethod call verification- Instances' equality check
- Can create & terminate multiple Firestore instances
- Can create and fetch documents from different Firestore instances offline and online