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

Add issues and issue_comments streams #9

Merged
merged 4 commits into from
Sep 11, 2021

Conversation

laurentS
Copy link
Contributor

This PR does a couple of things:

  • update the issues stream schema to match the docs (and actual API output) a bit more closely. I've not included all fields, namely excluding the "obvious" ones that can be computed trivially by appending a common postfix to the base url.
  • modify the issue_comments stream to be a child of repositories as discussed in Using different API endpoints for a "same" stream #8 and update the schema like above

Depending on how quickly this gets merged, I might add issue_events here, or in a separate PR, following the same logic.

th.Property("repo", th.StringType),
th.Property("org", th.StringType),
th.Property("issue_number", th.IntegerType),
th.Property("issue_url", th.IntegerType),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the issue_number property was previously coming from parent context. What do you think of adding back via post_process() method?

Something like "/".split(record["issue_url"])[-1] might do the trick.

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 like that! I'll add it in. It does seem handy to have the issue number available.

th.Property("updated_at", th.DateTimeType),
th.Property("created_at", th.DateTimeType),
# th.Property("closed_at", th.DateTimeType), # Nulls causing parse error
th.Property("closed_at", th.DateTimeType),
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for restoring this. I think the null behavior problem came up very early in testing and I forgot to retest and restore the field.

Copy link
Contributor

@aaronsteers aaronsteers left a comment

Choose a reason for hiding this comment

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

@laurentS - I think we can merge this quickly. Just one requested change regarding issue number on the comments stream. Let me know what you think. Thanks!

@laurentS
Copy link
Contributor Author

I've update the code as per your comments. I've been bothered by #11 but it's not critical for merging this, it just lets too much data through I think.

@aaronsteers
Copy link
Contributor

I've update the code as per your comments. I've been bothered by #11 but it's not critical for merging this, it just lets too much data through I think.

Responded on #11. Yes, I think that's an SDK issue and not needed to be resolved here necessarily. Thanks for raising.

Copy link
Contributor

@aaronsteers aaronsteers left a comment

Choose a reason for hiding this comment

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

Looks great! 👍

@aaronsteers aaronsteers merged commit 7107944 into MeltanoLabs:main Sep 11, 2021
@laurentS laurentS deleted the add-issues-streams branch September 11, 2021 12:16
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