Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the new dataconnect module, including the ConnectorConfig and DataConnect classes, along with a comprehensive test suite. However, the implementation is currently incomplete and will fail the tests. Specifically, the DataConnect class is missing the public app and config properties, the client function is left as a stub, and the _DataConnectService class is completely missing. Additionally, the validation in ConnectorConfig needs to be updated to explicitly check for string types to prevent invalid types from bypassing the checks.
|
it's important that PR/CL titles and descriptions are detailed enough so that reviewers (or future viewers) have the context they need to understand your changes will you check out go/cl-descriptions for some guidance and examples, and then add an updated description and title for the PR? |
stephenarosaj
left a comment
There was a problem hiding this comment.
looks gr8 - requested a few changes, mostly style or nits with one comment about code duplication
lahirumaramba
left a comment
There was a problem hiding this comment.
Thank you! Looks great! I added a few comments.
…d docstrings/synatx Refactored test suite to use module-level BASE_CONFIG and added parameterized client caching tests.
…ervice architecture Implemented ConnectorConfig dataclass, DataConnect client instance, _DataConnectService caching layer, and public client() factory function. Fixed mock recursion in test_client_successful using lambda delegation in test_data_connect.py.
…00/10 linter score
…est suite structure Removed duplicate client factory caching tests, moved app service loader test to unit test class, added connector property validation, and formatted parameter lists.
Added missing copyright headers and improved docstrings with proper formatting in dataconnect.py. Moved TestDataConnectServiceIntegration from tests/test_data_connect.py into a dedicated integration test file under integration/test_data_connect.py.
…st_data_connect.py Moved TestDataConnectServiceIntegration back to tests/test_data_connect.py and removed integration/test_data_connect.py.
…rkflow Renamed TestDataConnectServiceIntegration to TestDataConnectServiceWorkflow in tests/test_data_connect.py to clarify that it is an in-memory unit/functional test rather than a network integration test.
lahirumaramba
left a comment
There was a problem hiding this comment.
LGTM!
As we discussed offline, this requires a tech writer review before merging to the main branch.
stephenarosaj
left a comment
There was a problem hiding this comment.
LGTM (forgot to add before final review from Lahiru)
Feat: Implemented the foundational Firebase Data Connect Python SDK architecture (for Milestone 2) and their respective unit/integration tests:
This PR introduces a comprehensive test suite for the
firebase_admin.dataconnectmodule. The tests cover:ConnectorConfigclass behavior and validation.DataConnectclass initialization.dataconnect.client()factory function, including caching logic._DataConnectServiceclient management.This PR also includes the implementation of the core Data Connect module (
firebase_admin/dataconnect.py), designed to maintain architectural parity with the Node.js Admin SDK while leveraging idiomatic Python patterns:ConnectorConfig: Implemented as an immutable frozen dataclass (@dataclass(frozen=True)) with strict input validation that raises idiomaticValueErrorexceptions. Because it is frozen, Python automatically generates value-based hashing (__hash__), enabling clean object lookup.DataConnect: Implemented the primary client class with clean property accessors (.appand.config)._DataConnectService&client(): Built instance caching integrated directly into Python's FirebaseAppservice lifecycle (_utils.get_app_service), ensuring duplicate calls return cached instances.