Skip to content

Add support for assets endpoints#127

Merged
dsfish merged 9 commits intomasterfrom
df-assets
Jun 14, 2018
Merged

Add support for assets endpoints#127
dsfish merged 9 commits intomasterfrom
df-assets

Conversation

@dsfish
Copy link
Copy Markdown
Contributor

@dsfish dsfish commented Jun 5, 2018

No description provided.

@dsfish dsfish requested a review from joyzheng June 5, 2018 18:34
Comment thread plaid/requester.py Outdated
return response_body
else:
return response_body
return response.text
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since PDFs will be returned as bytes, I wonder if we should return response.content rather than response.text; thoughts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

⚡️

Comment thread plaid/requester.py Outdated
if response_body.get('error_type'):
raise PlaidError.from_response(response_body)

if is_json:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In the case that is_json=False but the endpoint returns an error, we're not going to enter this clause (and will end up returning the error as text). What do you think of making this check something like if is_json or response.headers['Content-Type']='application.json'?

@dsfish
Copy link
Copy Markdown
Contributor Author

dsfish commented Jun 6, 2018

@joyzheng, I ended up removing the is_json flag in favor of using the Content-Type header. LMK if you foresee any issues with this.

@joyzheng
Copy link
Copy Markdown
Contributor

joyzheng commented Jun 6, 2018

I ended up removing the is_json flag in favor of using the Content-Type header. LMK if you foresee any issues with this

Agree that the code is neater without is_json, however, I think it's still useful to explicitly expect json. In particular, there are edge error cases where we may get non-json (e.g. a load balancer error) and we want to make sure that we're throwing an error in that case, rather than returning (unexpected) non-json to the caller.

Maybe the solution here is to explicitly expect a content type in all cases then? (either json or pdf) and throw the error if that's not met.

David Fish added 3 commits June 13, 2018 16:06
This reverts commit c8dc014.
Comment thread plaid/requester.py Outdated
return response_body
else:
return response_body
return response.text
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

⚡️

Comment thread plaid/requester.py Outdated
if response_body.get('error_type'):
raise PlaidError.from_response(response_body)

if is_json and response.headers['Content-Type'] == 'application/json':
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@joyzheng, is this an OK compromise? I don't want to break any existing behavior, and I'm not sure if clients are using client.post anywhere.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, this looks good -- should be an or rather than an and here, though

@dsfish dsfish merged commit 1c6ecaf into master Jun 14, 2018
@dsfish dsfish deleted the df-assets branch June 14, 2018 19:00
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