Move last-30-days by default fix from the CLI to the client #518
Conversation
This fixes bugs in timelines and conversations, which use the search_all endpoint without the fix, therefore limiting them to the last 30 days only. This fix just moves the default start_time set in the CLI app to the client app, so it will apply by default to all uses of the search_all method.
|
Thanks for catching this @SamHames! It was just released in v2.4.2 |
|
Hmm I remember doing this for a reason - does this still work as expected with progress bars? |
|
@igorbrigadir I think you are the best judge of that. |
|
Ah, it does break it: For example: Is But is now also They're kinda hard to automate but these are the tests i did for the progress bars: The timelines ones did work as expected, but maybe there was another bug? I don't think i understand what the original error was in the first place:
The way it was previously, the API, |
|
Thanks @igorbrigadir. I apologize if I merged/released this too quickly. I'll create a new issue to address the desired progress bar behavior that you've documented here.
I think perhaps this is an area where the API behavior and twarc should diverge. As a user when I say: I expect to get all the conversation thread for tweet id 21. Not only the last 30 days. I think it's asking too much to expect twarc users to know that the default is the last 30 days and to remember to: While we can expect users of twarc as a library to be more knowledgeable about the Twitter API defaults, I think: should search all tweets by default...not just the last 30 days. But maybe I'm being wrongheaded? |
|
I opened this issue that hopefully characterizes the problem #519. I'll grab it since I introduced the problem by merging too quickly. |
|
I think in this case departing from the Twitter defaults and setting start
time to 2006 is less confusing (certainly to me anyway, it took ages to
figure out why there weren't many tweets coming through in timelines).
If we do decide to align with the Twitter API defaults then I think it
would be worth a pass to make sure that the documentation and config
options all clearly support the API defaults so there are fewer surprises.
…On Tue, 17 Aug 2021, 22:56 Ed Summers, ***@***.***> wrote:
Thanks @igorbrigadir <https://github.com/igorbrigadir>. I apologize if I
merged/released this too quickly. I'll create a new issue to address the
desired progress bar behavior that you've documented here.
The way it was previously, the API, client2.py behaved exactly as the API
docs - defaulting to last 30 days when start time was not set, and the
command2.py CLI had --archive option that set the start time for you - it
should have worked for conversations the same way, and timelines had
--use-search not --archive.
I think perhaps this is an area where the API behavior and twarc should
diverge. As a user when I say:
twarc2 conversation 21 --archive
I expect to get all the conversation thread for tweet id 21. Not only the
last 30 days. I think it's asking too much to expect twarc users to know
that the default is the last 30 days and to remember to:
twarc2 conversation 21 --archive --start-time 2006-03-21
While we can expect users of twarc as a library to be more knowledgeable
about the Twitter API defaults, I think:
client.search_all('fiddlesticks')
should search all tweets by default...not just the last 30 days. But maybe
I'm being wrongheaded?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#518 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACADAUKNWUBUIFHZHTOGKMDT5JL7ZANCNFSM5CJFTTGQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
|
Oh yeah, we should probably make a release/test check list or something to
check progress bars? I use them all the time and I still completely forgot
to check them 🙈
…On Tue, 17 Aug 2021, 23:18 Ed Summers, ***@***.***> wrote:
I opened this issue that hopefully characterizes the problem #519
<#519>. I'll grab it since I
introduced the problem by merging too quickly.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#518 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACADAUID2NTVB2JGFXFBG2DT5JOS3ANCNFSM5CJFTTGQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
|
I think the library should align with the API as close as possible - but the command line should be more user friendly. So these should work: (sets but should follow exactly how the API works: https://developer.twitter.com/en/docs/twitter-api/tweets/search/api-reference/get-tweets-search-all ( Alternatively, we can make will throw an will work - this way the command line and progress bars and stuff will work, and the library will also be clearer to use with a good error message. |
|
But making |
|
Yeah, I don't think twitter API is going to change so fast and often that we won't be able to keep up. I'd also prefer to stick to the API, but if we want to override |
This fixes bugs in timelines and conversations, which use the
search_all endpoint without the fix for the new API behaviour, therefore
limiting them to the last 30 days only even when using the academic
archive endpoints.
This fix just moves the default start_time set in the CLI app to
the client app, so it will apply by default to all uses of the
search_all method.