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

Updating singer_stream.py to allow schemas without properties #220

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

lucas
Copy link

@lucas lucas commented Aug 28, 2022

While utilizing tap-zendesk (https://github.com/twilio-labs/twilio-tap-zendesk) there are some cases where the generated schema does not include a properties key in the dictionary, causing the properties = self.schema['properties'] to fail.

Defaulting properties to an empty dictionary to prevent exception in these cases without unintended side effects.

While utilizing tap-zendesk (https://github.com/twilio-labs/twilio-tap-zendesk) there are some cases where the generated schema does not include a `properties` key in the dictionary, causing the `properties = self.schema['properties']` to fail.

Given that there is nothing returned from this function, returning early appears to resolve it.
Copy link
Collaborator

@laurentS laurentS left a comment

Choose a reason for hiding this comment

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

What do you think of the approach below instead?

target_postgres/singer_stream.py Outdated Show resolved Hide resolved
@lucas lucas requested a review from laurentS September 1, 2022 04:08
@laurentS
Copy link
Collaborator

Quick sense-check on this, is the absence of properties in the schema allowed by the singer spec? I took a quick look, but couldn't find a clear answer, in part because I don't have the data that causes problem. Basically, I'm wondering if the bug is on this end or in the tap you're using.

Updating dependencies
update arrow dep
@laurentS
Copy link
Collaborator

@lucas would you be able to merge the main branch into this PR to switch to the new dependency management format (pyproject.toml)?

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