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

refactor ExtractJsonArrayStringArgument #1

Open
eksperts opened this issue Mar 22, 2017 · 7 comments
Open

refactor ExtractJsonArrayStringArgument #1

eksperts opened this issue Mar 22, 2017 · 7 comments

Comments

@eksperts
Copy link
Collaborator

this here is the culprit: https://github.com/fusetools/Fuse.Billing.Android/blob/master/src/Fuse.Billing.Android/BillingModule.uno#L225

first, Json.Escape() is deprecated as of 0.36
second, the method seems poorly designed. it could be better if it would work with an array instead of if/elseing on the param type.

@kusma
Copy link

kusma commented Mar 22, 2017

Looking at the code, I can't really see how this code could ever have worked. It seems to try to create an array of a single string. But it calls Json.Escape, which doesn't put quotes around the string, making it an invalid JSON entity...

@eksperts
Copy link
Collaborator Author

well I know why this never occurred as a problem.
in my JS implementation, I'm always passing an array to this method, even if it's just a single SKU. So effectively in my case that part of the method was never triggered.

@kusma
Copy link

kusma commented Mar 22, 2017

Right, that makes some kind of sense.

@BeeWarloc
Copy link
Collaborator

@eksperts If I understand you correctly you mean it would be more straight-forward if the method only accepted an array? I think I agree with that.

@kusma Yeah, you're right. The Json.Escape thing here definitively never worked, it should rather have been Json.Stringify instead (if that accepts a string). Could be that the author is a bit of an idiot, not actually testing the code he wrote. 😸 ✋

@eksperts
Copy link
Collaborator Author

@BeeWarloc you understood me correctly, yes

@LuisRodriguezLD
Copy link
Member

Hello. Any news on this?

@BeeWarloc
Copy link
Collaborator

@LuisRodriguezLD Nope, unfortunately haven't gotten to this yet.

The fix should be quick and simple, but I'm leaning towards just changing the documentation to say it just supports taking in an array of skus, as I think the string overload just makes the API more ambiguous. 🙂

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

No branches or pull requests

4 participants