chore(firestore): prep for firestore pipelines GA by daniel-sanche · Pull Request #16549 · googleapis/google-cloud-python
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request removes preview API warnings from several Firestore pipeline modules and adds support for an optional options dictionary in the raw_stage method of the pipeline. Unit tests were also added to verify the new functionality. Feedback was provided regarding a missing type hint import for Value in base_pipeline.py and the placement of imports within test functions, which should ideally be moved to the top of the files per PEP 8.
| self, | ||
| name: str, | ||
| *params: Expression, | ||
| options: dict[str, Expression | Value] | None = None, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type hint Value is used here but it does not appear to be imported in this file. If Value is a type alias defined in google.cloud.firestore_v1.pipeline_expressions, it should be imported alongside Expression to avoid a NameError at runtime, especially if type hints are evaluated.
Comment on lines +454 to +455
| from google.cloud.firestore_v1.base_vector_query import Field | ||
| from google.cloud.firestore_v1.pipeline_stages import RawStage |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is generally better practice to place imports at the top of the file rather than inside a test function, unless there is a specific reason to defer the import (e.g., avoiding circular dependencies). Also, consider if Field should be imported from google.cloud.firestore_v1.pipeline_expressions for consistency with the pipeline API being tested.
References
- PEP 8: Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants. (link)
Comment on lines +443 to +444
| from google.cloud.firestore_v1.base_vector_query import Field | ||
| from google.cloud.firestore_v1.pipeline_stages import RawStage |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the async pipeline tests, these imports should ideally be moved to the top of the file. Additionally, ensure that importing Field from base_vector_query is intentional and consistent with how it's used in the pipeline implementation.
References
- PEP 8: Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants. (link)