Kebechet / Maui.RevenueCat.InAppBilling

MIT License
12 stars 6 forks source link

error handling for null values #4

Closed CameronVetter closed 1 year ago

CameronVetter commented 1 year ago

This fixes #2 by adding handling for null values coming back from the native Java code.

Kebechet commented 1 year ago

Thanks for the PR.

Regarding changes. What do you think about following thing. Instead of making those fields nullable, edit extension method ToDateTime so that it can accept nullable Date (right now it accepts just normal one and that is probably the main problem).

And even ManagementURL is nullable ?

CameronVetter commented 1 year ago

Thanks for the PR.

Regarding changes. What do you think about following thing. Instead of making those fields nullable, edit extension method ToDateTime so that it can accept nullable Date (right now it accepts just normal one and that is probably the main problem).

And even ManagementURL is nullable ?

I see what you mean, that is in the spirit of the extension method. I do think it would be better if the extension returned a nullable with a default value instead of a minvalue, that is a bit of a "magic number" I'll update the PR.

CameronVetter commented 1 year ago

Thanks for the PR.

Regarding changes. What do you think about following thing. Instead of making those fields nullable, edit extension method ToDateTime so that it can accept nullable Date (right now it accepts just normal one and that is probably the main problem).

And even ManagementURL is nullable ?

Yes customerInfo.ManagementUrl is null in my case. ToString throws an exception if this is null, and since this is the default ToString this either needs to be handled the way I did it, or a new extension method needs to be created to handle it. Let me know what you think and I'll update the PR.

Kebechet commented 1 year ago

Ok, this is much better. I went through iOS implementation and it wasnt 1:1. As you can see here: https://github.com/Kebechet/Maui.RevenueCat.InAppBilling/blob/main/Maui.RevenueCat.InAppBilling/Platforms/iOS/RevenueCatBillingiOS.cs#L221-#L229

So to keep it unified please:

after these changes I will merge it and release a new version.

CameronVetter commented 1 year ago

Ok, this is much better. I went through iOS implementation and it wasnt 1:1. As you can see here: https://github.com/Kebechet/Maui.RevenueCat.InAppBilling/blob/main/Maui.RevenueCat.InAppBilling/Platforms/iOS/RevenueCatBillingiOS.cs#L221-#L229

So to keep it unified please:

  • Android

    • for ManagementURL please use null propagation like on iOS
    • in JavaDateExtensions please return null instead of default. It is a minor thing, but I prefer it this way to have it explicitly set
  • iOS

after these changes I will merge it and release a new version.

Agreed, once I get it tested ill update the PR.