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

Respect the schema passed in the stream. #29

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

Conversation

cgardens
Copy link

@cgardens cgardens commented Oct 2, 2020

  • Extract headers from schema object and use those headers to write to the CSV.

  • Write tests that flatten and get_headers produce the same keys.

Description of change

Currently this integration does not use schema to set headers in the CSV files. It instead relies on what is already in the file or the keys it can find in the record. I believe this is the source of these issues #3 and #22. My understanding of the singer spec is that it would be appropriate to use the schema records to make sure the correct headers are used in the CSV files, instead of sampling records.

Manual QA steps

  • Ran against the exchangeratesapi tap to sanity check that everything still worked. Wrote tests to check the specific behavior around missing headers. Happy to do more if this PR gains traction.

Risks

  • This change should be save, because if the record does not comply with the schema, the integration throws an error already. It will change the output in the cases where sampling the record was leading to incomplete headers, so it is technically a breaking change, but I think it is only breaking in cases that were already pretty broken.

Rollback steps

  • revert this branch

* Extract headers from schema object and use those headers to write to the CSV.

* Write tests that flatten and get_headers produce the same keys.
@cmerrick
Copy link
Contributor

cmerrick commented Oct 2, 2020

Hi @cgardens, thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes.

@cmerrick
Copy link
Contributor

cmerrick commented Oct 2, 2020

You did it @cgardens!

Thank you for signing the Singer Contribution License Agreement.

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.

4 participants