Skip to content

Fix apple payload datamodel access#14

Merged
MihaelIsaev merged 2 commits intoMihaelIsaev:masterfrom
h2glab:fix_apple_payload_datamodel_access
Nov 28, 2019
Merged

Fix apple payload datamodel access#14
MihaelIsaev merged 2 commits intoMihaelIsaev:masterfrom
h2glab:fix_apple_payload_datamodel_access

Conversation

@FredericRuaudel
Copy link
Copy Markdown
Contributor

@FredericRuaudel FredericRuaudel commented Nov 28, 2019

In order to test my model properties, I need to access the properties values of the APNs data models.

As they are already public in the Android side, I imagine this is not a deal breaker to homogenize it on the Apple side?

I just have a few doubts on the final result:

  1. should we make them accessible only in readonly mode with a private(set) (I didn't add it since Android is fully public)
  2. To make this work, I had to make FCMApnsAlertOrString public to make Swift compiler happy in FCMApnsApsObject (aka: can't set property public because type used in not public yadiyada)
  3. Not sure on the naming of the FCMApnsAlertOrString helpers to access the enum associated values: alertPayload/alertMessage or asPayload/asMessage or asPayload/asString

at call site it looks like this right now:

iosConfig?.payload.aps.alert?.alertPayload?.titleLocArgs

so maybe it would look better like this:

Config?.payload.aps.alert?.asPayload?.titleLocArgs

🤔

What do you think?

Thanks in advance!

@MihaelIsaev MihaelIsaev merged commit bf42d59 into MihaelIsaev:master Nov 28, 2019
@MihaelIsaev
Copy link
Copy Markdown
Owner

MihaelIsaev commented Nov 28, 2019

@FredericRuaudel thank you for the pull request! ❤️

  1. Maybe it'd look better, yeah 🙂 but we definitely should keep old way too just by marking it deprecated to save backward compatibility

@FredericRuaudel
Copy link
Copy Markdown
Contributor Author

FredericRuaudel commented Nov 28, 2019

Maybe it'd look better, yeah 🙂 but we definitely should keep old way too just by marking it deprecated to save backward compatibility

I'm not sure we understand each other here, I've added those computed properties so I could have change the name right now and no need for deprecated marker. Or I didn't get what you said? 🤔

@MihaelIsaev
Copy link
Copy Markdown
Owner

MihaelIsaev commented Nov 28, 2019

Maybe it'd look better, yeah 🙂 but we definitely should keep old way too just by marking it deprecated to save backward compatibility

I'm not sure we understand each other here, I've added those computed properties so I could have change the name right now and no need for deprecated marker. Or I didn't get what you said? 🤔

Yeah, my bad, sorry I was in a hurry 🙂 no questions! all good! 🙂

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@FredericRuaudel @MihaelIsaev