Support all standard params for filter, sample statuses and user events #165
Conversation
Codecov Report
@@ Coverage Diff @@
## master #165 +/- ##
==========================================
+ Coverage 92.4% 92.42% +0.02%
==========================================
Files 69 70 +1
Lines 1119 1122 +3
Branches 16 16
==========================================
+ Hits 1034 1037 +3
Misses 85 85
Continue to review full report at Codecov.
|
|
Hi @allantl for your awesome PR! Overall look ok, but I cannot merge it because of the fix on locations -- it's a breaking change. Any chance you could separate the fix for location in a separate PR? This way we could release the rest in the next release without any breaking change |
| */ | ||
| def filterStatuses(follow: Seq[Long] = Seq.empty, | ||
| tracks: Seq[String] = Seq.empty, | ||
| locations: Seq[Double] = Seq.empty, | ||
| locations: Seq[(Double, Double)] = Seq.empty, |
There was a problem hiding this comment.
Probably we should use a case class here rather than simple tuples....but how should we call it? we already have a case class Location (that is quite different) and used in the REST Client
There was a problem hiding this comment.
There are some improvements that can be made here. Looking at the official twitter java client which is quite outdated here, seems that each bounding box location must be in the form of (southwest coordinate, northeast coordinate).
We can create case classes similar to the link above. Not sure about the name though. For reference, elasticsearch has similar usecase.
Also, looking at this doc, most coordinates are in the form of [LONG,LAT], except geo attribute [LAT, LONG], we can improve case classes such as Area, Coordinates. It would be another breaking change.
There was a problem hiding this comment.
I wouldn't worry too much about breaking changes: as long as we document them and that they are done with reason, then people are gonna be fine with it. The problem is that we need to do them, bump up the version and properly document them.
We have a few requests already that we know are probably gonna need API breaking changes, for example see #142 and #144.
I am gonna move this discussion into a separate issue (see #169) so that we can discuss it more easily.
|
Yes sure, I'll create another PR for the locations fix. |
|
@allantl could you please rebase this PR and maybe separate the location fix in another PR? Thanks! |
|
@DanielaSfregola All done! |
This PR :
Partially addresses Support all standard params for Streaming requests #71. Reference: Basic stream parameters
As I don't have access to both of these stream endpoints, Im not confident to modify it since I can't do the UAT.
Plus I can't find the documentation for this two endpoints.
FYI, User streams and Site streams will be deprecated some time in the future. See here.
Fix Clarify TwitterStatusClient scaladoc #130
I don't think we should mention
comma-separatedin the docs since that is basically hidden behind the API.Also, I removed most of the links which are not active anymore.
Fix locations parameters
It needs to be a list of longitude,latitude pairs. I added the encoder for list of tuples as well.