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

Add canMakePayments #52

Merged
merged 10 commits into from
Jun 30, 2021
Merged

Add canMakePayments #52

merged 10 commits into from
Jun 30, 2021

Conversation

beylmk
Copy link
Contributor

@beylmk beylmk commented Jun 16, 2021

Adds canMakePayments method

Opening in draft because I still need to remove the aars and clean up some formatting, but ready for review on the actual implementation.

@beylmk beylmk changed the title Initial implementation with local hybrid-common Add canMakePayments Jun 16, 2021
@@ -18,7 +18,8 @@ public partial class Purchases : MonoBehaviour
public delegate void GetOfferingsFunc(Offerings offerings, Error error);

public delegate void CheckTrialOrIntroductoryPriceEligibilityFunc(Dictionary<string, IntroEligibility> products);


public delegate void CanMakePaymentsFunc(bool canMakePayments, Error error);
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we don't have docs for the other functions, but I think we should start adding them (and add for all the other functions on a Friday). Do you mind adding docs here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vegaro created clubhouse ticket for the others: https://app.clubhouse.io/revenuecat/story/7152/add-documentation-to-unity-functions, as well as a ticket for researching/setting up unit testing (if you could add comments on that tool you saw, that'd be great...): https://app.clubhouse.io/revenuecat/story/7153/research-set-up-unity-unit-tests

Copy link
Contributor

@vegaro vegaro left a comment

Choose a reason for hiding this comment

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

looks good so far, I will re review when the tmp files are cleaned up

@@ -17,8 +17,7 @@ private class ProductsRequest
{
public string[] productIdentifiers;
}

[DllImport("__Internal")]
[DllImport("__Internal")]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change is unintended

@@ -29,6 +28,7 @@ public void GetProducts(string[] productIdentifiers, string type = "subs")

_RCGetProducts(JsonUtility.ToJson(request), type);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

extra line

@beylmk beylmk marked this pull request as ready for review June 17, 2021 22:04

void _RCCanMakePayments(const char *featuresJSON) {
NSError *error = nil;
NSDictionary *canMakePaymentsRequest = [NSJSONSerialization JSONObjectWithData:[convertCString(featuresJSON) dataUsingEncoding:NSUTF8StringEncoding] options:0 error:&error];
Copy link
Member

Choose a reason for hiding this comment

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

let's add a line break here for readability, either by extracting the CString conversion into its own line or by doing a line per parameter

@@ -0,0 +1,72 @@
// GENERATED BY UNITY. REMOVE THIS COMMENT TO PREVENT OVERWRITING WHEN EXPORTING AGAIN
Copy link
Member

Choose a reason for hiding this comment

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

was checking in this file intended?

@beylmk
Copy link
Contributor Author

beylmk commented Jun 22, 2021

@vegaro @aboedo i believe this is ready for re-review

try {
object.put("canMakePayments", canMakePayments);
} catch (JSONException e) {
e.printStackTrace();
Copy link
Member

@aboedo aboedo Jun 23, 2021

Choose a reason for hiding this comment

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

why the difference in error handling between here and line 324354?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, no real reasoning just copy pasta. i'll update so they're consistent 👍

Copy link
Contributor

@vegaro vegaro left a comment

Choose a reason for hiding this comment

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

Looks good! There's a Subtester/Assets/Plugins/Android/mainTemplate.gradle.DISABLED file that I am not sure is intended to be there :)

JSONObjectWithData:[convertCString(featuresJSON)
dataUsingEncoding:NSUTF8StringEncoding]
options:0
error:&error];
Copy link
Contributor

Choose a reason for hiding this comment

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

The formatting of these lines is a little bit off

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there seemed to be a really wide array of formattings for this kinda call... this felt the most readable to me, but open to suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

In objc, : are aligned if the parameters are in different lines. Apart from that, maybe extracting the data into a variable also helps. Something like this :

NSData *data = [convertCString(featuresJSON) dataUsingEncoding:NSUTF8StringEncoding];
NSDictionary *canMakePaymentsRequest = [NSJSONSerialization JSONObjectWithData:data
                                                                       options:0
                                                                         error:&error];

/// </summary>
/// <param name="features">An array of BillingFeatures to check for support.
/// If empty, no features will be checked.</param>
/// <param name="callback">A callback receiving a bool for canMakePayments and potentially an Error</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

Great, love these docs thanks!!!

@beylmk beylmk merged commit b6330af into develop Jun 30, 2021
@beylmk beylmk deleted the can-make-payments branch June 30, 2021 00:30
@beylmk beylmk mentioned this pull request Jun 30, 2021
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.

3 participants