Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Set XML Accept header when fetching tabular data #66

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jwalgran
Copy link

The default initialization for instances of this class sets the Accept header to application/json. The transform_tabular_data utility function that is used to parse the response expects XML content.

Connects #62

PR #64 includes a similar fix as part of a larger change set. If #64 is merged this PR can be closed.

The default initialization for instances of this class sets the Accept header to
application/json. The transform_tabular_data utility function that is used to
parse the response expects XML content.
@anttipalsola
Copy link

@jwalgran Is there some reason to use XML in the first place? As the BambooHR API is also capable to return tabular data as JSON, why not just skip utils.transform_tabular_data altogether and just return r.json()?

@jwalgran
Copy link
Author

The PR as written is the smallest change I could come up with that prevented the function from raising an exception. I tend to agree that since the data is being transformed and returned as a dictionary, requesting the tabular data as JSON from the start makes sense. I am not sure if the data returned from the API has the same "shape" as the dictionary currently returned by transform_tabular_data. If it does not, we would presumably want to repurpose transform_tabular_data and reshape the JSON so that the values returned from get_tabular_data remain consistent.

@anttipalsola
Copy link

The "shape" of the JSON data roughly as follows:
[[{User 1 row 1}, {User 1 row 2}], [{User 2 row 1}], ..., [{User N row 1}, ..., {User N row N}]]

So, it is not identical. I agree that we should keep the API stable, so I did what you suggested and created yet another fix for the same problem: #67. I did not modify transform_tabular_data because it is used also by other functions but created a new one (transform_json_tabular_data).

So, now we have three PRs for the same problem. I hope one of them gets merged soon. :-)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants