refactor: split firestore_event_data into proto_event_data and domain… - #300
refactor: split firestore_event_data into proto_event_data and domain…#300IzaakGough wants to merge 1 commit into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request refactors the Firestore endpoint handler in firestore_fn.py by removing redundant type casts, renaming event data variables, and introducing domain_event_data to represent the parsed domain event. Feedback highlights two key issues: first, domain_event_data is declared but not initialized, which could lead to an UnboundLocalError if the event type is unmatched; second, removing the type cast on the event handler call will trigger static type checking failures, so the cast should be restored.
Sorry, something went wrong.
| proto_event_data.old_value.update_time, | ||
| ) | ||
|
|
||
| domain_event_data: DocumentSnapshot | Change[DocumentSnapshot | None] | None |
There was a problem hiding this comment.
The variable domain_event_data is declared with a type annotation but is not initialized with a default value. If event_type does not match any of the expected values in the conditional blocks, referencing domain_event_data later (e.g., on line 202) will raise an UnboundLocalError. Initializing it to None ensures safer and more robust execution.
| domain_event_data: DocumentSnapshot | Change[DocumentSnapshot | None] | None | |
| domain_event_data: DocumentSnapshot | Change[DocumentSnapshot | None] | None = None |
Sorry, something went wrong.
| else: | ||
| # mypy cannot infer that the event type is correct, hence the cast | ||
| _typing.cast(_C1 | _C2, func)(database_event) # type: ignore[arg-type] | ||
| func(database_event) |
There was a problem hiding this comment.
Removing the type cast and type: ignore will cause static type checkers (like mypy or pyright) to fail. Since func can be of type _C3 or _C4 (which expect an AuthEvent), passing database_event (which is a standard Event) is a type mismatch. The cast to _C1 | _C2 is necessary to inform the type checker that func is a non-auth event handler in this branch.
| func(database_event) | |
| _typing.cast(_C1 | _C2, func)(database_event) # type: ignore[arg-type] |
Sorry, something went wrong.
Reviewers
1 more reviewerAt least 1 approving review is required to merge this pull request.
Follow up to #297