Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Do not try to cast every value of JSON to integer #21

Merged
merged 1 commit into from
Oct 8, 2015
Merged

Do not try to cast every value of JSON to integer #21

merged 1 commit into from
Oct 8, 2015

Conversation

morgoth
Copy link

@morgoth morgoth commented Oct 2, 2015

JSON response should have proper type already set. Trying convert values to integer leads to bugs like for "last4" attribute with "0042" that would be converted to 42

Relates to #18

There are still failing tests because of hardcoded ids of resources -> #9

Also I found that paymill API is returning amount as strings in some cases which is a bug considering the documentation. Should I report it somewhere else?

JSON response should have proper type already set. Trying convert values to integer leads to bugs like for "last4" attribute with "0042" that would be converted to 42
@morgoth
Copy link
Author

morgoth commented Oct 2, 2015

rebased with current master

@nikoloff
Copy link

nikoloff commented Oct 8, 2015

Hi @morgoth

any ideas why is this PR fails on travis-ci?
I run it locally and it doesn't fail.
I would like to merge it into paymill master.

@morgoth
Copy link
Author

morgoth commented Oct 8, 2015

@nikoloff Looks like there is no API key set on travis?

@nikoloff
Copy link

nikoloff commented Oct 8, 2015

I put the keys there (the master branch build is green). Will check it again, 10x

@morgoth
Copy link
Author

morgoth commented Oct 8, 2015

Probably secret keys are not set on PR builds

@nikoloff
Copy link

nikoloff commented Oct 8, 2015

can't see the problem, will try to merge it locally

@nikoloff nikoloff merged commit bc23f57 into paymill:master Oct 8, 2015
@morgoth morgoth deleted the fix-json-value-casting branch October 8, 2015 08:38
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants