Skip to content

Rationalize C++ source layout #446

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

Merged
merged 5 commits into from
Jun 9, 2021
Merged

Rationalize C++ source layout #446

merged 5 commits into from
Jun 9, 2021

Conversation

ehsannas
Copy link
Contributor

@ehsannas ehsannas commented Jun 4, 2021

See b/174877213 for details and doc.

@ehsannas
Copy link
Contributor Author

ehsannas commented Jun 4, 2021

I have broken the PR down to smaller contained commits that can be reviewed.

The last commit is just the result of running the scripts/format_code.py script.

Copy link
Contributor

@var-const var-const left a comment

Choose a reason for hiding this comment

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

A couple of points:

grep still shows a few mentions of _ios (including those in comments):

$ grep -RI _ios firestore

./integration_test_internal/src/field_value_test.cc:#include "firestore/src/ios/field_value_ios.h"
./integration_test_internal/src/util/integration_test_util.cc:#include "firestore/src/ios/firestore_ios.h"
./integration_test_internal/src/firestore_test.cc:  // Also, the logic in `util_ios.h` can be modified to make sure that
./src/android/field_value_android.h:  // these constructors must match the equivalents in field_value_ios.h.

(omitting some irrelevant stuff)

I would urge you to import this change before merging it (I think Copybara should be able to handle an unmerged branch) and doing all the necessary changes to the BUILD files, just in case it uncovers any issues with this PR.

Consider reverting the formatting changes, they increase the delta without being directly related to the theme of this PR. I'd prefer to have those committed via a separate PR.

@var-const var-const assigned ehsannas and unassigned var-const Jun 4, 2021
@ehsannas ehsannas force-pushed the ehsann-fix-source-layout branch from 9507062 to 18fd68a Compare June 4, 2021 19:36
@ehsannas
Copy link
Contributor Author

ehsannas commented Jun 4, 2021

(1) Uh, that's a great catch! Thanks. I have addressed things that were found via grep. Done.
(3) I have dropped the formatting commit (so the check_format bot will fail). Done.
(2) I'll focus on this, and once I'm done I'll assign the PR back to you.
Thanks

@ehsannas ehsannas added the tests-requested: quick Trigger a quick set of integration tests. label Jun 4, 2021
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jun 4, 2021
@ehsannas
Copy link
Contributor Author

ehsannas commented Jun 4, 2021

I got a successful import (a few minor changes were needed to internal files) via cl/377574378

@github-actions github-actions bot added the tests: failed This PR's integration tests failed. label Jun 4, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 4, 2021
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests: failed This PR's integration tests failed. labels Jun 9, 2021
@github-actions
Copy link

github-actions bot commented Jun 9, 2021

✅  Integration test succeeded!

Requested by @ehsannas on commit cdb07c1
Last updated: Wed Jun 9 10:06:10 PDT 2021
View integration test results

@ehsannas
Copy link
Contributor Author

ehsannas commented Jun 9, 2021

The integration test failures seemed random/flakey 😢 (and different tests were failing on different platforms). I've restarted the GitHub Actions...

@github-actions github-actions bot added the tests: succeeded This PR's integration tests succeeded. label Jun 9, 2021
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 9, 2021
@ehsannas
Copy link
Contributor Author

ehsannas commented Jun 9, 2021

Integration tests passed 👍

@ehsannas ehsannas merged commit 7b9742a into main Jun 9, 2021
@ehsannas ehsannas deleted the ehsann-fix-source-layout branch June 9, 2021 17:09
@firebase firebase locked and limited conversation to collaborators Jul 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes tests: succeeded This PR's integration tests succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants