Skip to content
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

feat(playstore): add pointer in DeveloperNotification #268

Merged
merged 5 commits into from
Mar 19, 2024
Merged

Conversation

jaryf
Copy link

@jaryf jaryf commented Mar 16, 2024

add pointer for SubscriptionNotification, OneTimeProductNotification, VoidedPurchaseNotification, TestNotification in DeveloperNotification

…ductNotification, VoidedPurchaseNotification, TestNotification in DeveloperNotification
Copy link
Member

@takecy takecy left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Why do you want to pointer only here?

Currently, the reason we are not making the field a pointer is so that users don't have to check if it is nil or not.

@jaryf
Copy link
Author

jaryf commented Mar 16, 2024

Thanks for the PR!

Why do you want to pointer only here?

Currently, the reason we are not making the field a pointer is so that users don't have to check if it is nil or not.

If these three parameters are not set as pointers, it won't be possible to determine if they are nil. So, in that case, how can I determine the type of notification received?

@takecy
Copy link
Member

takecy commented Mar 16, 2024

Thanks for your response.
For example, it would be, checking if the Version field is empty.

var noti DeveloperNotification

/// received

if noti.OneTimeProductNotification.Version != "" {
  // this is OneTime Notification
}

@jaryf
Copy link
Author

jaryf commented Mar 17, 2024

Thanks for your response. For example, it would be, checking if the Version field is empty.

var noti DeveloperNotification

/// received

if noti.OneTimeProductNotification.Version != "" {
  // this is OneTime Notification
}

However, according to the official documentation, these fields are actually mutually exclusive. So, my idea is to use pointers to determine if they are nil, which can also determine the type of notification. If we were to judge whether a specific field inside is an empty string, it would lead to misunderstanding. Moreover, each person may judge different fields, which would lack standardization.

@takecy
Copy link
Member

takecy commented Mar 17, 2024

I understood 👍
However, this struct is already exported and we do not want to force existing users to deal with it.
Therefore, instead of making changes to the existing struct, how about commenting it as Deprecated and redefining it as a separate struct?

@jaryf
Copy link
Author

jaryf commented Mar 18, 2024

I understood 👍 However, this struct is already exported and we do not want to force existing users to deal with it. Therefore, instead of making changes to the existing struct, how about commenting it as Deprecated and redefining it as a separate struct?

Okay, I'll try to make some modifications. Thank you for your response. It's my first time submitting a PR on GitHub. If there's anything I've done wrong, please feel free to point it out.

@jaryf
Copy link
Author

jaryf commented Mar 18, 2024

I understood 👍 However, this struct is already exported and we do not want to force existing users to deal with it. Therefore, instead of making changes to the existing struct, how about commenting it as Deprecated and redefining it as a separate struct?

I added a struct DeveloperNotificationNew. Can you see if this works?

// DeveloperNotificationNew is sent by a Pub/Sub topic.
// Detailed description is following.
// https://developer.android.com/google/play/billing/rtdn-reference#json_specification
type DeveloperNotificationNew struct {
Copy link
Member

Choose a reason for hiding this comment

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

Can it be DeveloperNotificationV2 as New may cause confusion at a later date? 🙏

Copy link
Author

Choose a reason for hiding this comment

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

Can it be DeveloperNotificationV2 as New may cause confusion at a later date? 🙏

ok

@@ -38,6 +38,7 @@ const (
// DeveloperNotification is sent by a Pub/Sub topic.
// Detailed description is following.
// https://developer.android.com/google/play/billing/rtdn-reference#json_specification
// Deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Deprecated comments should be Deprecated: .
For example, Depreacated: use DeveloperNotificationV2 intead.
It would be kind to do it this way ☺️
https://go.dev/wiki/Deprecated

Copy link
Author

Choose a reason for hiding this comment

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

ok

Copy link
Member

@takecy takecy Mar 19, 2024

Choose a reason for hiding this comment

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

Depreacated: use DeveloperNotificationV2 intead.
A colon + space is required after the Deprecated character.
And please write on one line 🙏

In this format, the editors will issue a warning.

@jaryf
Copy link
Author

jaryf commented Mar 19, 2024

Thank you for your correction. I have modified it. Please check whether it complies with the specifications.

// DeveloperNotificationV2 is sent by a Pub/Sub topic.
// Detailed description is following.
// https://developer.android.com/google/play/billing/rtdn-reference#json_specification
type DeveloperNotificationV2 struct {
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -38,6 +38,7 @@ const (
// DeveloperNotification is sent by a Pub/Sub topic.
// Detailed description is following.
// https://developer.android.com/google/play/billing/rtdn-reference#json_specification
// Deprecated
Copy link
Member

@takecy takecy Mar 19, 2024

Choose a reason for hiding this comment

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

Depreacated: use DeveloperNotificationV2 intead.
A colon + space is required after the Deprecated character.
And please write on one line 🙏

In this format, the editors will issue a warning.

@jaryf
Copy link
Author

jaryf commented Mar 19, 2024

I have modified it. Please help me check whether it conforms to the specifications. Thank you.

Copy link
Member

@takecy takecy left a comment

Choose a reason for hiding this comment

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

Thank you for responding to my detailed points.
LGTM 👍

@@ -38,6 +38,7 @@ const (
// DeveloperNotification is sent by a Pub/Sub topic.
// Detailed description is following.
// https://developer.android.com/google/play/billing/rtdn-reference#json_specification
// Depreacated: use DeveloperNotificationV2 instead.
Copy link
Member

Choose a reason for hiding this comment

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

👍

@takecy takecy merged commit 734cc17 into awa:master Mar 19, 2024
2 checks passed
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