-
Notifications
You must be signed in to change notification settings - Fork 252
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
v2 Output Format Options #112
Conversation
this commit just as a shortcut to fork the v2 branch as a reminder to myself
I wasn't really sure if that's the way |
the geo expansion was wrong, it's not a list. fixed. |
There's a more compact, alternative implementation here which i really like: https://github.com/geduldig/TwitterAPI/blob/master/TwitterAPI/TwitterAPI.py#L414 it replaces the json structure as opposed to appending extra data, which i felt was important to keep the "atomic" output still compatible with any other tools that expect the non atomic format (it does not expand user mentions because those are referenced by |
on second thought: |
OK, I need to take a look soon. Thanks for all the information. |
I think i will change that polls thing afterall.. when i get back home later today. |
Might have another try at the replacements part of it, I think I have a better way of doing that too, just going to double check that it works and doesn't blow up 😅 |
I cleaned up the code a bit more, i think it's working now the way i imagined it - tweets are expanded with whatever extra info is available in that individual call. One caveat that maybe should be documented somewhere: some objects like geo places or pinned tweets can be left unexpanded because the place id or tweet wasn't inside includes on the same "page". Keeping all the expansions in memory across calls i thought would not be a good idea. |
More thinking out loud: Another thing i was thinking about was the possibility of specifying multiple output formats - or alternatively, including the raw request as an extra line of output in the "atomic" variant - just like the current version writes out the meta and includes objects separately, the "atomic" one proposed here should maybe write out the raw request as a separate line too. Sure, this means that it's writing nearly double the data, but i think it's worth preserving because |
Hi Igor, Pondering these options: 'a' - atomic, 'r' - response, 'c' - constructed" I like 'atomic' and 'response', but 'constructed' is not resonating with me ;) Trying to think of a more descriptive name for the mode where the client writes out the "data", "includes" and "meta" arrays in series. Can you provide more context for your comments on the "errors"? Are these embedded errors about supporting objects that could not be provided? I am not convinced about providing for simultaneous output modes... If the forcing function for that is some loss of "errors" metadata, let's update the code to handle those, rather than support simultaneous modes...? |
Yeah - i just made that one the current default to be backwards compatible i guess, so it wouldn't break anything that's currently relying on this format. Maybe "decomposed" is a better term for it? I actually think "response" format should be the default - 1 json line per original API response makes a lot of sense to have as the default, and then optionally "atomic" outputting each tweet as an option.
Yes - these are the errors included in As for multiple output formats, it's more of an idea, I don't have strong enough opinions or requirements to do that to warrant working on it more - the raw "response" format should be the one to use in that case. |
Digging back into the code this week. In the process of testing the three output options. The default 'r' option looks good so far, I like how it just echos back the exact response. Moving on to 'a' testing... finding that the command-line setting is not being passed through, so looking into that. Also, it seems that the currently separate '--atomic' option could/should be dropped in favor of a single "--output-option" (will rename it to be singular) option. Also adding in a one-second pause when hitting the "all" endpoint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After merging this pull request, will be testing more and making updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will take another pass while testing and update.
Great! I fixed that command line issue in #121 |
@igorbrigadir So, in my testing (inside my IDE) I don't see the atomic Tweets until all are "ready." So it is not yielding/emitting the Tweets one by one, but rather there is nothing emitted until the entire 'plug' of Tweets is ready... Different behavior from a few iterations ago. Maybe it is just me. I need to test on the command line next. |
Following on from #102 (comment) i added some options to output "atomic" format of tweets, with all expansions filled inline in the tweet results.
I assumed "constructed" format would be the format that's currently implemented, and the new proposed "atomic" one has all includes inline. "response" format, i took to mean the raw response from the API.
Let me know what you think.