-
Notifications
You must be signed in to change notification settings - Fork 1
twitter : new stream to extract tweets tagging us #35
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I added some small remarks
source-twitter-fetcher/Dockerfile
Outdated
@@ -1,5 +1,5 @@ | |||
FROM airbyte/python-connector-base:1.1.0@sha256:dd17e347fbda94f7c3abff539be298a65af2d7fc27a307d89297df1081a45c27 | |||
|
|||
#FROM airbyte/python-connector-base:1.1.0@sha256:dd17e347fbda94f7c3abff539be298a65af2d7fc27a307d89297df1081a45c27 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove it
@@ -11,7 +11,7 @@ data: | |||
connectorType: source | |||
definitionId: 1c448bfb-8950-478c-9ae0-f03aaaf4e920 | |||
dockerImageTag: '1.0.0' | |||
dockerRepository: harbor.status.im/bi/airbyte/source-twitter-fetcher | |||
dockerRepository: harbor.status.im/bi/airbyte/source-twitter-fetcher |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you add a whitespace at the end ?
description: "List of Twitter handles to monitor (e.g., ['@IFT', '@Airbyte'])" | ||
items: | ||
type: string | ||
minItems: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why minimum 1 item ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess i will never know :'(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no tag the connector will not work the "for tag in self.tags" loop never executes also the goal of the connector is to look for tags it doesn't make sense to have it empty that's why it is also a mandatory parameter
|
||
logger = logging.getLogger("airbyte") | ||
|
||
class TagsStream(HttpStream): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use the TwitterStream
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason, you're right using twitterstream is better
def next_page_token(self, response: requests.Response) -> Optional[Mapping[str, Any]]: | ||
if 'meta' in response.json() and 'next_token' in response.json()['meta'] and response.json()['meta']['result_count'] > 0: | ||
logger.debug('DBG-NT: %s', response.json()['meta']['next_token']) | ||
return {"next_token": response.json()['meta']['next_token']} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be mutualized in TwitterStream
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you thing ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree !
@@ -8,88 +8,31 @@ | |||
|
|||
logger = logging.getLogger("airbyte") | |||
|
|||
class TagsStream(HttpStream): | |||
class TwitterStream(HttpStream): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why redefined it ?
It already exist in TwitterStream.py https://github.com/status-im/airbyte-custom-connector/blob/master/source-twitter-fetcher/source_twitter_fetcher/tweets_stream.py
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have delete a bit to much no ?
Without the parse_response it wont work
Related to this issue : https://github.com/status-im/data-docs/issues/36
You can chose any tag you are interested in in the source config in airbyte
Related airbbyte connection