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

[APIC-306] Fixed a bug in parsing responses that included "update" as a key #410

Merged
merged 1 commit into from
Sep 14, 2020

Conversation

mheilman
Copy link
Contributor

This fixes a bug where a column named "update" would cause response parsing to fail for /tables endpoints.

The civis.response.Response provides convenient access to response attributes (e.g., response.attrib as opposed to response['attrib']). In retrospect, perhaps it'd have been better not to have that functionality recur all the way down the response structure (or at least not into user-provided table info), but that ship has sailed, and in order to minimize breaking changes, this PR should preserve the existing functionality and avoid the bug.

…., in column information from `client.tables.get(...)`)
@@ -174,3 +174,15 @@ def test_convert_data_type_civis_list():
assert isinstance(data[0], Response)
assert data[0]['foo'] == 'bar'
assert data[0].headers == {'header': 'val'}


def test_parse_column_names():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test would fail before


self.update(self_updates)
# Update self.__dict__ at the end to avoid replacing the update method.
self.__dict__.update(self_updates)
Copy link
Contributor

Choose a reason for hiding this comment

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

so the issue is that the update key was overwriting the dictionary update method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, and then the next update call would fail.

Copy link
Contributor

@salilgupta1 salilgupta1 left a comment

Choose a reason for hiding this comment

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

LGTM

@mheilman mheilman merged commit f580251 into civisanalytics:master Sep 14, 2020
@mheilman mheilman deleted the fix-response-parsing branch September 14, 2020 17:53
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