Skip to content

Conversation

@rhiannon-eldridge-lrn
Copy link
Contributor

@rhiannon-eldridge-lrn rhiannon-eldridge-lrn commented Aug 2, 2019

Fixes #34

Checklist

  • Feature

  • Bug

  • Security

  • Documentation

  • ChangeLog.md updated

  • Tests added

  • All testsuites passed

  • make dist completed successfully

@rhiannon-eldridge-lrn rhiannon-eldridge-lrn changed the title Cat 213/bug/fix results iter for object responses [BUG] Fix issue with results_iter when API returns an object CAT-213 Aug 2, 2019
@rhiannon-eldridge-lrn rhiannon-eldridge-lrn changed the title [BUG] Fix issue with results_iter when API returns an object CAT-213 [BUG] Fix #34 - results_iter when API returns an object CAT-213 Aug 5, 2019
@rhiannon-eldridge-lrn rhiannon-eldridge-lrn changed the title [BUG] Fix #34 - results_iter when API returns an object CAT-213 [BUG] fix #34 - results_iter when API returns an object CAT-213 Aug 5, 2019
@rhiannon-eldridge-lrn rhiannon-eldridge-lrn changed the title [BUG] fix #34 - results_iter when API returns an object CAT-213 [BUG] fixes #34 - results_iter when API returns an object CAT-213 Aug 5, 2019
@flakeparadigm flakeparadigm changed the title [BUG] fixes #34 - results_iter when API returns an object CAT-213 [BUG] Fix results_iter when API returns an object CAT-213 Aug 5, 2019

def _build_base_url(self):
@staticmethod
def __build_base_url():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to a static method as it's only used internally by the integration tests and does not access any class variables.

Copy link
Contributor

@flakeparadigm flakeparadigm left a comment

Choose a reason for hiding this comment

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

👍 We'll continue without the CI approvals because it's a config issue in Travis. #36 should cleanup this error, but fine for now.

for result in response['data']:
yield result
if isinstance(response['data'], dict):
for key, value in Future.iteritems(response['data']):
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for Future:

Suggested change
for key, value in Future.iteritems(response['data']):
for key in response['data']:
yield {key: response['data'][key]}

shtrom
shtrom previously requested changes Aug 5, 2019
Copy link
Contributor

@shtrom shtrom left a comment

Choose a reason for hiding this comment

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

I don't think we need to add this Future helper.

Copy link
Contributor

@shtrom shtrom 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. Haven't funced.

@shtrom shtrom dismissed their stale review August 6, 2019 01:16

Addressed.

@shtrom
Copy link
Contributor

shtrom commented Aug 6, 2019

We'll have to remember to remove the iteritems boilerplate when we deprecate Python2.

@rhiannon-eldridge-lrn rhiannon-eldridge-lrn merged commit dce77a4 into master Aug 7, 2019
@rhiannon-eldridge-lrn rhiannon-eldridge-lrn deleted the CAT-213/bug/fix-results-iter-for-object-responses branch August 7, 2019 01:35
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.

Incorrect response data handling

4 participants