Made LookupFriendship accept multiple users - #290

_ParseAndCheckTwitter should raise a ValueError when the response is not a json string, for instance if Twitter throws a fail whale, or if the json response contains an error block, then it should raise a TwitterError since this means there was something wrong with the call to the API like bad authentication data or a malformed request. You're right not to wrap it in a try block since these should be surfaced to the end user.

@jeremylow should we merge this PR and then fix your items ourselves?

Nothing that needed to be fixed from my side. The comment about parsing was
just a clarification about how errors from twitter get raised. I guess my
only concern is the addition of a keyword argument specifically for 'users'
since a couple other methods use instance checking to determine whether to
join a list or pass through as a string. I think keeping the arguments for
methods to a minimum is probably for the best and most future-proof, but if
this is consistent with the majority of other places where this kind of
thing crops up, my personal preference as a consumer of APIs is to value
consistency over the alternative.

On Wed, Feb 10, 2016, 11:50 Mike Taylor notifications@github.com wrote:

@jeremylow https://github.com/jeremylow should we merge this PR and
then fix your items ourselves?

—
Reply to this email directly or view it on GitHub
#290 (comment).

Agree that we should always aim for consistency as that is what consumers are looking at (and for)

I think I ended up adding the 'users' kwarg just because it was also in the UsersLookup method. I also found it was actually super useful when using the api, as I'd get a whole bunch of twitter.User objects from GetFriends, and having to iterate through those and take just the username felt pretty unnecessary when the users kwarg was accepted in other lookup methods on python-twitter.

@andkon - then let's merge and get this into the system!

:D thanks folks!

Sorry to be late to this, but I just took a closer look at the changes and I think this changes some previous behavior (which is fine since there are a bunch of other breaking changes for 3.0), but maybe should be considered:

Previously, user_id and screen_name didn't do any type checking, so if you wanted to look up a bunch of users, you could pass a string in the form of (api.LookupFriendship(user_id="120292,1920301,12301") or an int (api.LookupFriendship(user_id=12)) and that would get passed to Twitter unchanged. This also meant that you could pass a single user as an int and you'd get a response. Behavior now is to only accept a list; I'm not sure how widespread the previous version was used, but it might warrant a closer look.

The screen_name parameter could also accept @andkon 's changes so that the way the keyword arguments get constructed is consistent (i.e., you could pass a list of twitter.User objects and concatenate them on their user.screen_name parameter). (Also, as it stands, if you pass a single string like screen_name="jack", consistent with previous behavior, you get something like "j,a,c,k" so that needs at least type checking to make sure the user passes a list-like iterable.)

I wonder if it would be better to remove the users parameter and retain user_id and screen_name and then use isinstance checking to see if User objects get passed to either one of those and construct the query string being passed to Twitter from those. @bear @andkon thoughts?

The only other thing that I see is that the return type changes depending on whether one or many users are returned. I would prefer to see one return type, e.g., a list, even if it only contains one entry.

I'm happy to work on the changes above if they're a good idea. I wrote up the changes as-is for the migration to 3.0 along with some tests against the endpoint, so I can integrate with that if it's OK. Again, sorry to be late to the game on the above.

I'm +1 to both of those suggestions @andkon @jeremylow