Closed ConnorSinnott closed 3 years ago
"This interlinked system makes the piece-by-piece introduction of the V2 model difficult, but it can be done by prohibiting requests for fields or expansions which reference data not yet modeled in the code." ... 👍 it makes sense!
So far is looking really good!
There is some duplication, but it is well marked and, tbh, it will be very difficult to avoid -- so I am ok with it.
I look forward to see how this PR will evolve...so far, it looks very good! :)
Hey @DanielaSfregola! I'm marking this as review ready now that its been successfully capturing the tweets I've been running through it. Unfortunately Twitter's documentation doesn't clearly represent which properties are optional, and additionally I'm seeing some extra fields being returned by the live tests which aren't listed in the API docs, so I expect there will be a few more tweaks needed as we go.
Since you last viewed:
I've pulled the V2 endpoints into its own "V2" REST client and removed the "V2" prefix from the new classes. I'm thinking this would be a more future-proof approach should V1 ever become deprecated.
Instant
s coming from Twitter now assume the more traditional form of 2021-06-11T18:30:12.000Z
, so the override of EEE MMM dd HH:mm:ss ZZZZ yyyy
was colliding. I've changed the parsing of Instants to lazily iterate through both the old parser and Instant.parse
and return the first one to succeed.
I've expanded the mock data to better represent a fully materialized Tweet. I was running into a few false-negatives from the test suite when the JSON parser wasn't configured right. If the same JSON parser is responsible for materializing the REST results as well as materializing the "expected" result, an error in the parser would cause both objects to be equally erroneous and the test will pass. I was thinking of checking equality against hand-written case classes, but I wanted to run that by you first.
Writing the case classes by hand is certainly better -- the only reason why I didn't do it at the time is that I was developing 50+ endpoints and it wasn't practical for particularly verbose endpoints.
Thanks for the speedy reviews! I've changed the signature of parseInstant
back based on the Try.getOrElse
approach suggested (neat). Additionally, I spent a few minutes creating the case class representation of these tweet responses and found that coordinates weren't being deserialized correctly 😅. So I've popped in a new custom serializer for Tuple2[Double, Double]
.
One thing to note is that I've placed the scala fixtures into a fixtures
package within their relative tests.
- src/test/scala/com/danielasfregola/twitter4s/http/clients/rest/v2
- tweets
- fixtures
- TweetResponseFixture.scala
- TweetsResponseFixture.scala
- TwitterTweetLookupClientSpec.scala
This is a bit of a departure from your structure, but the root fixtures
directory isn't setup for scala. Let me know if I should try something else.
I threw in one more last-minute change: The errors
field of TweetsResponse
and TweetsResponse
are now Seq[Error]
instead of Option[Seq[Error]]
. The thinking is that other parts of this response are optional because they must be requested. The errors field isn't optional and will always be returned if they exist. So this change reflects that intention in the code.
This PR adds partial support for the Tweet Lookup endpoints under Twitter's new V2 API.
Twitter's new Object Model consists of 5 objects:
In a GraphQL-esque style, a query for Tweets under a tweet endpoint can accept a series of "Expansions" and "Fields" to be materialized in the result. In this way, a query for recent tweets can include in its response fully materailized Users, Media, and Polls based on the fields requested.
This interlinked system makes the piece-by-piece introduction of the V2 model difficult, but it can be done by prohibiting requests for fields or expansions which reference data not yet modeled in the code.