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

sampled stream functionality #57

Closed
wants to merge 3 commits into from
Closed

Conversation

ChrisTorresLugo
Copy link
Member

This pull request contains functionality for sampled stream endpoint.

A summary of the modifications contained in this PR are:

  • oauth.py: modifies OAuth2's make_request parameters and requests.get to add a stream parameter that enables the creation of a streaming connection.

  • api.py: creates a public interface for sampled_stream endpoint and its corresponding private implementation. These functions are used to establish a connection to the API and to return a generator that is used by the user for streaming purposes.

This endpoint can be tested using the following code:

bearer_token = ""
oauth2 = osometweet.OAuth2(bearer_token=bearer_token)
ot = osometweet.OsomeTweet(oauth2)

response = ot.sampled_stream(everything=True)

for tweet in response:
    print(json.loads(tweet))

Documentation and tests will follow on a different PR if we are ok with this implementation.

@ChrisTorresLugo ChrisTorresLugo added the enhancement New feature or request label Feb 17, 2021
@ChrisTorresLugo ChrisTorresLugo added this to the Basic functionality milestone Feb 17, 2021
@ChrisTorresLugo ChrisTorresLugo linked an issue Feb 17, 2021 that may be closed by this pull request
3 tasks
Changed default value of stream parameter in OAuth2's make_request function to False, instead of None.
@mr-devs
Copy link
Member

mr-devs commented Feb 17, 2021

Documentation and tests will follow on a different PR if we are ok with this implementation.

Sounds good. Thanks!

@yang3kc
Copy link
Member

yang3kc commented Mar 3, 2021

Opps, I think I messed this PR up...

@mr-devs
Copy link
Member

mr-devs commented Mar 3, 2021

Opps, I think I messed this PR up...

Something other than the conflicts?

@yang3kc
Copy link
Member

yang3kc commented Mar 3, 2021

Just the conflicts, which I think I just fixed.

However, I'm now sure if the sampled_stream method can properly work with the rate limit manager. You might want to take a look.

@yang3kc
Copy link
Member

yang3kc commented Mar 3, 2021

Also, this PR seems to be reverting some of the previous commits from other PRs. I didn't touch them because I'm not sure which one we want.

@mr-devs
Copy link
Member

mr-devs commented Mar 4, 2021

Also, this PR seems to be reverting some of the previous commits from other PRs. I didn't touch them because I'm not sure which one we want.

Sorry if this is obvious but could you be more specific?

@@ -171,7 +171,8 @@ def make_request(
response = requests.get(
Copy link
Member

Choose a reason for hiding this comment

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

@mr-devs I was talking about line 171 to 181. Here the requests response is a "stream" object, then it was fed into the manage_rate_limits function. Not sure if this would cause conflicts.

Copy link
Member

Choose a reason for hiding this comment

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

I see. I am not super familiar with the "stream" object so it's hard to say.

It seems like the first try block within rate_limit_manager.py would throw an error and then log an exception which would likely be incorrect because it is designed for the typical response object. Also, the next conditional may cause an error if there is no status_code object within the "stream".

@ChrisTorresLugo do you think you can let us know your thoughts on this whenever you get a chance? One option is to create another function within rate_limit_manager.py that is called only when the request is for a stream. There is probably a more elegant way, though.

Keep in mind that the rate limit function will need to be rewritten anyway because it currently misses some errors caused by the bug in Twitter's system.

@yang3kc
Copy link
Member

yang3kc commented May 9, 2021

Sorry I haven't been able to keep up with this thread for a while. What's the status?

Also, there seem to be some conflicts with the master branch. Please resolve.

@mr-devs
Copy link
Member

mr-devs commented May 11, 2021

Sorry I haven't been able to keep up with this thread for a while. What's the status?

Also, there seem to be some conflicts with the master branch. Please resolve.

I think we are waiting on @ChrisTorresLugo's thoughts on how the stream would interact with the rate_limit_manager. Right now the rate_limit_manager is working fine (I addressed the above-mentioned bugs from a while back) and handles Twitter's quirks, however, I'm sure that it could be cleaned up a bit.

@ChrisTorresLugo
Copy link
Member Author

Matt is correct. I'll get back to this next week.

@mr-devs
Copy link
Member

mr-devs commented Jul 13, 2021

hey @ChrisTorresLugo - any update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Samples Stream Endpoint
3 participants