-
Notifications
You must be signed in to change notification settings - Fork 34
fix: preserve crashlytics options #279
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
base: main
Are you sure you want to change the base?
Changes from all commits
4dc650e
8663261
e3dba27
7eb1eb8
724a1bb
fbd9359
0359eb4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -652,6 +652,27 @@ def _endpoint( | |
| ) | ||
|
|
||
|
|
||
| def _alert_options_to_firebase_alert_options( | ||
| options: EventHandlerOptions, | ||
| alert_type: str | AlertType, | ||
| ) -> FirebaseAlertOptions: | ||
| app_id = getattr(options, "app_id", None) | ||
|
|
||
| # Restrict to fields supported by FirebaseAlertOptions | ||
| allowed_fields = {f.name for f in _dataclasses.fields(FirebaseAlertOptions)} | ||
|
|
||
| option_values = { | ||
| field.name: getattr(options, field.name) | ||
| for field in _dataclasses.fields(options) | ||
| if field.name in allowed_fields and field.name not in {"alert_type", "app_id"} | ||
| } | ||
| return FirebaseAlertOptions( | ||
| **option_values, | ||
| alert_type=alert_type, | ||
| app_id=app_id, | ||
| ) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there can be a possibility of a future regression here. If a developer adds a custom options field to a specific subclass (e.g., AppDistributionOptions) that is not present on FirebaseAlertOptions, the current implementation will include it in option_values and attempt to pass it to the FirebaseAlertOptions constructor. This will raise a runtime error. Maybe we can try to filter option_values to only include fields defined in firebaseAlertOptions? Ai suggested something like this: def _alert_options_to_firebase_alert_options(
options: EventHandlerOptions,
alert_type: str | AlertType,
) -> FirebaseAlertOptions:
app_id = getattr(options, "app_id", None)
# Restrict to fields supported by FirebaseAlertOptions
allowed_fields = {f.name for f in _dataclasses.fields(FirebaseAlertOptions)}
option_values = {
field.name: getattr(options, field.name)
for field in _dataclasses.fields(options)
if field.name in allowed_fields and field.name not in {"alert_type", "app_id"}
}
return FirebaseAlertOptions(
**option_values,
alert_type=alert_type,
app_id=app_id,
)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! I think the suggested snippet here looks good. Will add this. |
||
|
|
||
|
|
||
| @_dataclasses.dataclass(frozen=True, kw_only=True) | ||
| class AppDistributionOptions(EventHandlerOptions): | ||
| """ | ||
|
|
@@ -669,9 +690,9 @@ def _endpoint( | |
| **kwargs, | ||
| ) -> _manifest.ManifestEndpoint: | ||
| assert kwargs["alert_type"] is not None | ||
| return FirebaseAlertOptions( | ||
| alert_type=kwargs["alert_type"], | ||
| app_id=self.app_id, | ||
| return _alert_options_to_firebase_alert_options( | ||
| self, | ||
| kwargs["alert_type"], | ||
| )._endpoint(**kwargs) | ||
|
IzaakGough marked this conversation as resolved.
|
||
|
|
||
|
|
||
|
|
@@ -692,9 +713,9 @@ def _endpoint( | |
| **kwargs, | ||
| ) -> _manifest.ManifestEndpoint: | ||
| assert kwargs["alert_type"] is not None | ||
| return FirebaseAlertOptions( | ||
| alert_type=kwargs["alert_type"], | ||
| app_id=self.app_id, | ||
| return _alert_options_to_firebase_alert_options( | ||
| self, | ||
| kwargs["alert_type"], | ||
| )._endpoint(**kwargs) | ||
|
IzaakGough marked this conversation as resolved.
|
||
|
|
||
|
|
||
|
|
@@ -715,9 +736,9 @@ def _endpoint( | |
| **kwargs, | ||
| ) -> _manifest.ManifestEndpoint: | ||
| assert kwargs["alert_type"] is not None | ||
| return FirebaseAlertOptions( | ||
| alert_type=kwargs["alert_type"], | ||
| app_id=self.app_id, | ||
| return _alert_options_to_firebase_alert_options( | ||
| self, | ||
| kwargs["alert_type"], | ||
| )._endpoint(**kwargs) | ||
|
IzaakGough marked this conversation as resolved.
|
||
|
|
||
|
|
||
|
|
@@ -733,8 +754,9 @@ def _endpoint( | |
| **kwargs, | ||
| ) -> _manifest.ManifestEndpoint: | ||
| assert kwargs["alert_type"] is not None | ||
| return FirebaseAlertOptions( | ||
| alert_type=kwargs["alert_type"], | ||
| return _alert_options_to_firebase_alert_options( | ||
| self, | ||
| kwargs["alert_type"], | ||
| )._endpoint(**kwargs) | ||
|
|
||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.