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

implemented NextTweetsAsync for GetTweetsFromUserId #46

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

Conversation

bmclane
Copy link

@bmclane bmclane commented Dec 14, 2022

No description provided.

@Xwilarg
Copy link
Owner

Xwilarg commented Dec 14, 2022

The PR itself looks good to me but the main issue is that it would break the compatibility with the previous version
What do you think of it, @marcogruhl ?

@bmclane
Copy link
Author

bmclane commented Dec 14, 2022

if breaking compatibility is a big issue here, could add this, but was attempting to keep the next token functionality the same for User and Tweet Lists:
public Func<Task<Tweet[]>> LoadNextTweetsAsync;
and set the function inside GetTweetsFromUserIdAsync,

LoadNextTweetsAsync = data.Meta.NextToken == null ? null : async () => await NextTweetsAsync(query, data.Meta.NextToken, Endpoint.UserTweetTimeline);

@marcogruhl
Copy link
Collaborator

The PR itself looks good to me but the main issue is that it would break the compatibility with the previous version What do you think of it, @marcogruhl ?

It looks good to me too. It extends the functionality and make it more consistent with the other functions. That said, i dont like, that only one function is "updated". If there are breaking changes we should do it for all functions with next_token consistently. Maybe also do it more general. E.g. Combine NextTweetsAsync and NextUsersAsync into one function with generic return. But this can be done later without breaking changes ...

marcogruhl
marcogruhl previously approved these changes Dec 15, 2022
Copy link
Collaborator

@marcogruhl marcogruhl left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Maybe correct double empty lines or missing empty lines.

@marcogruhl
Copy link
Collaborator

marcogruhl commented Dec 16, 2022

I just came across the idea to use an optional out parameter for the next token to avoid breaking changes. not completely aware if this works. i remember similar discussions for the rateLimits. That said, this PR could be the start of a new major release and i will refactor the rateLimit functionality!? So every answer had data, ratelimit and data (array or object)
@bmclane sorry for the unconviniances
@Xwilarg what do you think about the major release idea?

@Xwilarg
Copy link
Owner

Xwilarg commented Dec 16, 2022

We could use that to make a new major, it would be interesting to see if there is any other breaking change we want to push with it tho

@bmclane
Copy link
Author

bmclane commented Dec 16, 2022

No worries @marcogruhl, I added the previous token to finish the next/previous token handling.

@marcogruhl
Copy link
Collaborator

@Xwilarg where to go from here? 3.0 branch for breaking PRs?

@Xwilarg
Copy link
Owner

Xwilarg commented Dec 27, 2022

I guess we can make a new branch for the 3.0 and merge it there instead of master

@marcogruhl
Copy link
Collaborator

Happy new year!

between the days i found some time to try some ideas for V3. you can look at it under my branch (based on @bmclane #46 ): GeneralRequest

My intention for this is to make a "general request" like GetEndpoint(id) => Request(id, Endpoint) so we can implement every endpoint with mostly one line of code.

I would define the primary goals for V3:

  • Full App-Scope implementation
  • RateLimit response with every request

secondary goals:

  • Full User-Scope implementation
  • 3-legged OAuth

so far:

  • Full tweet endpoints implementation
  • working general requests
  • less breaking implementation of @bmclane PR (inherit from List for Page/Response (RArray) object
  • problems with user scope (auth)
  • remove RateLimit eventhandler or keep it in parallel?

Any thoughts about this?

@Xwilarg
Copy link
Owner

Xwilarg commented Jan 1, 2023

Happy New Year, I think it's a good idea

I'm also using this topic to ask you if you would like to speak directly about that on some others platform such as Discord, I think it would help to move things quicker especially on a topic such as this one that goes over the scope of the current PR

@marcogruhl
Copy link
Collaborator

marcogruhl commented Jan 1, 2023

Happy New Year, I think it's a good idea

I'm also using this topic to ask you if you would like to speak directly about that on some others platform such as Discord, I think it would help to move things quicker especially on a topic such as this one that goes over the scope of the current PR

send you a friend request. maybe we can open a channel with @bmclane and others (later hopefully 😅)

@jamescarter-le
Copy link
Contributor

Looking forward to having usage of the next_time for my use case.
If there is a branch for this I'm happy to trial this in my own app.

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