agent-twitter-client for rust #152
Conversation
|
lol my bad, deleted Cargo.toml by mistake. please check it again |
There was a problem hiding this comment.
This is quite a hefty PR and that's resulted in a fairly complex review. I've done my best to highlight as much feedback as I could w/o repeating myself.
This crate is essentially a translation of a Typescript library -> Rust. While the library was adapted fairly well to compile in Rust, it's design and structure is not very Rust natural, making it a bit awkward to use from a DX perspective. This library also relies on a lot of custom hardcoded logic to work with the Twitter API and has a bit of a unique authorization flow that can make it tricky to replicate in different environments. This complexity leads me to believe that this might want to exist in it's own repo / crate or perhaps a very slimmed down version of this could sit on-top of an existing bindings implementation. A rig-twitter crate would likely sit on top of that providing pre-built tools and vector integrations to make building twitter agents even more seamlessly.
Top-down Suggestions
This crate does a good job of encapsulating a lot of behavior into the Scaper struct and implementation. I actually started out confused looking at the Client before I realized that the Scraper is the intended entrypoint. To me, it feels like Scraper should be considered the main Client from the dev POV and all services and state should be managed by the client (passing itself to helper methods or other implementations in other subcrates).
- I would have a
Clientstruct that defers state and behavior to the other files / subcrates. - This
Clientshould be created with a singularreqwests::Clientthat is reused across all REST requests (see the provider clients inrig-core, a.newmethod usually auto-inits one but a dev could manually create a client if they wanted to reuse areqwestclient.
It might be suitable to look into a graphql crate for the graphql endpoints. This crate in particular lets you design the GraphQL query via serde structs which gives you a fully typed response, great DX!
The authentication seems a bit odd, I think some diagraming would be really helpful to map out. There's a lot of stuff going on with cookie management, auth handling with log ins, and more.
Overall, this is a really thorough PR and would be an excellent addition to the rig ecosystem, whether or not it lives within the main rig repo or as a side-repo. I also think there's opportunities to further enhance this integration by bundling pre-made tools and agent builders for people to work and extend. Let me know if you have any questions, I'd be happy to walk through my review a bit more even directly on Discord!
rig-twitter/Cargo.toml
Outdated
| regex = "1.5" | ||
| url = "2.5.0" | ||
| async-stream = "0.3" | ||
| futures-core = "0.3" |
There was a problem hiding this comment.
Some of these dependencies could be optional or considered dev dependencies. See the rig-core/cargo.toml.
Highlights:
toktio
Unless this crates is requiring specifically tokio usage, it should be required. The main rig-core crate is runtime agnostic and that should likely be matched to other rig crates unless a reason is provided.
dotenv
As per rig-core/cargo.toml, this is generally just used in examples. It can be added as a dev dependency, but is probably not needed as a core dep.
async-stream and futures-core
I don't think these are used / need to be defined (might be already included by a different crate in the .lock). I commented them out, the package still builds.
Double check the other dependencies to make sure they are necessary (and not just used accidentally added). A quick 1 liner about some of the deps would be helpful for future maintainers for this crate!
There was a problem hiding this comment.
This crates is requiring toktio so i cant remove it
- Refactored `rig-twitter` module to improve structure and functionality, including: - Updated authentication handling to use `user_auth::TwitterAuth`. - Enhanced the `Scraper` struct to utilize a shared `TwitterApiClient`. - Modified various functions to accept a `Client` parameter for better request handling. - Removed unused example file and image. - Updated README.md for clarity and accuracy. - Improved error handling and logging throughout the module.
|
Thank you so much for your detailed response about my code. I have fixed it based on your instructions. I know it looks a bit rough, but you still took the time to review it and provide very detailed guidance, and I truly appreciate that. Most of your comments have been addressed. Regarding the BEARER_TOKEN in the constants.rs file, it's the Twitter API's required bearer token, not the user's, so I can't put it in the .env file. I have successfully merged the new rig-twitter into Rina as well. Thanks again! |
Ofc, happy to support new contributors! Btw, it's helpful to click "Resolved" or leave a note on the existing comments I've made so I can see that they've been accounted for. Or you can leave a comment / question that I can address. I'll take a further look at this soon. In terms of whether this twitter agent should exist in it's own repo or how it fits into the ecosystem, I'll come back and talk with the team to figure that out! Regardless, I'll still review and make suggestions to the best of my abilities! |
- Updated `get_tweet` function to return the first tweet found or an error if none exist. - Removed `ReferencedTweet` and `ReferencedTweetKind` structures from `tweets.rs` as they were unused. - Cleaned up `parse_timeline_tweet` and `parse_legacy_tweet` functions by eliminating the `referenced_tweet` field, streamlining the codebase.
|
very excited to see what can be done with agentic social content creation in rust! looking forward to seeing a twitter agent in action! |
Hey, you could check out Rina's GitHub repo. She's using Rig-Twitter and can interact on Twitter, Telegram, and Discord too https://github.com/cornip/Rina |
There was a problem hiding this comment.
The improvements you made are good. One thing, you've passed around the reqwest client and twitter auth around but my main suggestion was to use your own TwitterClient as the source of truth to be used by the subcrates for the operations.
w.r.t where this crate should live. there's nothing specific to rig or llms in this crate atm, it's all just sitting on top of the twitter API and general cookie and auth flow. I think it would be more fitting for this to live in a agent-twitter-client crate that you can use for your own Rina project and for others to install alongside it.
I also saw that you've published this as rig-twitter on crates.io, I think that's a bit misleading bc nothing in that crate that directly uses rig — it doesn't even use it as a dependency. It would be best if it was renamed to agent-twitter-client. A rig-twitter crate would be a set of tools or features that integrates with rig agents to interact w/ twitter.
It could easily sit on top of this API or the other twitter-v2 framework. As it stands rn, I don't think we'll be merging this into main but I think it should definitely live on your end.
Twitter API not required. All details are provided in the README: https://github.com/cornip/rig-twitter/blob/main/README.md