-
Notifications
You must be signed in to change notification settings - Fork 125
Firestore build and testing improvements and documentation #1113
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
Conversation
❌ Integration test FAILEDRequested by @wu-hui on commit 0cac295
Add flaky tests to go/fpl-cpp-flake-tracker |
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.
This looks super helpful.
I noticed a few problems with wording and additions I would like to see.
|
||
It is easier to work this Firestore CPP SDK by keeping a high level archetecture in mind: | ||
|
||
 |
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.
I am not sure where this diagram came from, but instinctively the arrows seem to point in the wrong direction. I looked up UML diagrams, and how IntelliJ does a Dependency Diagrams (also based on UML I think), and they all seem to point towards the root of dependency.
The first question that popped into my mind when I saw this diagram was: Why does Core SDK have knowledge of Swift? Shouldn't Swift just be an agnostic wrapper around Core SDK? Then I realized that my understanding of arrows (which is consistent with UML) is completely backwards to what the diagram uses.
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.
Addressed. I am never a fan of UML, but I guess there is value to follow known conventions.
firestore/CONTRIBUTING.md
Outdated
|
||
## IDE Integration | ||
|
||
Open up the repo root directory from `CLion` should load all symbols for the SDK itsel should |
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.
There are some repeated words:
should load all symbols for the SDK itsel
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.
Fixed.
firestore/CONTRIBUTING.md
Outdated
## IDE Integration | ||
|
||
Open up the repo root directory from `CLion` should load all symbols for the SDK itsel should | ||
load all symbols for the SDK itself. Once loaded, you can right load on |
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.
I think right load
should be right click
? What option from drop down menu should I choose?
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.
Fixed.
firestore/CONTRIBUTING.md
Outdated
|
||
# Android building and testing | ||
|
||
Once Android NDK is installed and added to `local.properties`, and `google-services.json`, |
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.
Where can I find more instruction on installing Android NDK? If I recall, there is a specific version that must be used.
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.
local.properties should be the same as you use for Android. Or is there anything in particular?
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.
@tom-andersen Please take another look.
🍞 Dismissed stale approval on external PR.
No description provided.