Skip to content

refactor: split firestore_event_data into proto_event_data and domain…#300

Draft
IzaakGough wants to merge 1 commit into
mainfrom
@invertase/refactor-firestore_event_data
Draft

refactor: split firestore_event_data into proto_event_data and domain…#300
IzaakGough wants to merge 1 commit into
mainfrom
@invertase/refactor-firestore_event_data

Conversation

@IzaakGough

@IzaakGough IzaakGough commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Follow up to #297

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

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 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.

proto_event_data.old_value.update_time,
)

domain_event_data: DocumentSnapshot | Change[DocumentSnapshot | None] | None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

Suggested change
domain_event_data: DocumentSnapshot | Change[DocumentSnapshot | None] | None
domain_event_data: DocumentSnapshot | Change[DocumentSnapshot | None] | None = None

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

Suggested change
func(database_event)
_typing.cast(_C1 | _C2, func)(database_event) # type: ignore[arg-type]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants