Support all available parameters for status methods (with fix)

[GoogleCodeExporter]

After reading issue 22
(http://code.google.com/p/python-twitter/issues/detail?id=22), I noticed
that several twitter api parameters aren't supported with this module (not
just since_id).  I'm not sure, but I suspect that is because those
parameters were added to the twitter API after this module was written.

I've uploaded a patch that with fix issue 22 with forward computability for
future twitter api parameters.  The patch allows pethods in this module
accept parameters as a dictionary, instead of individually.  For example:

timeline = twitter.Api().GetUserTimeline(user = sodio,
parameters={'since_id': '932442808'})

instead of:

timeline = twitter.Api().GetUserTimeline(user = sodio, since_id = 932442808)

This will support all existing parameters while maintaining forward
compatibility with future parameters that are supported by the twitter API
.

The patch also maintains backwards compatibility with pre-existing
installations.  All tests pass (expect for the 2 that we already know
about).  Someone might want to create new tests that make use of the
parameters dictionary.

Modified methods:
- GetPublicTimeline
- GetFriendsTimeline
- GetUserTimeline

Original issue reported on code.google.com by sam%odio...@gtempaccount.com on 27 Sep 2008 at 7:51

Attachments:

• parameters.patch

[GoogleCodeExporter]

Sorry for the typos, I didn't really proofread what I was writing :)

I also modified the GetDirectMessages in the same way as above.

Original comment by sam%odio...@gtempaccount.com on 27 Sep 2008 at 7:55

[GoogleCodeExporter]

Thanks for the patch!  I definitely support the goal of supporting all 
parameters.

But is a dictionary keyed by strings the best approach?  What if we had 
enumerated
constants for each key?  Something like:

parameters = { Parameters.SINCE_ID: 932442808,
               Parameters.COUNT: 10,
               # ... }

That said, I think I'd still prefer that we just update the method signatures
directly.  That's far more discoverable, don't you think?

-DeWitt

Original comment by dclinton on 27 Sep 2008 at 8:06

[GoogleCodeExporter]

While leaving it as-is might be easier for the developer and make sense now, 
the code
will require regular maintenance over time.  I'm not sure if it'll happen 
(that's not
intended to be an insult to you - I know this is an open source project and 
you're
only one person).  For example, issue 22 has been around for months, and I'm 
pretty
sure twitter changed their api months before that.

While I probably wouldn't want to do this, passing the parameter dictionary 
isn't
necessarily mutually exclusive with the existing implementation.  Maintaining 
both
would be somewhat inelegant, but it'd at least allow users to make use of new
parameters as soon as twitter releases them.  Users looking for basic API usage 
could
still continue to use python-twitter in its current form.

Also - I'm not an expert in python but it seems like passing arguments within a
dictionary seems relatively common.  For example, django does it in their url
dispatcher:
http://docs.djangoproject.com/en/dev/topics/http/urls/?from=olddocs#passing-extr
a-options-to-view-functions

Original comment by sam%odio...@gtempaccount.com on 27 Sep 2008 at 10:44

[GoogleCodeExporter]

Not a bad point -- they aren't mutually exclusive.  We could do something like:

  GetUserTimeline(user = 'foo',
                  since_id = 932442808,
                  extra_parameters = { 'count': 10 })

And everything passed in as 'extra_parameters' would automatically be included 
in the
request.

Sounds good?

Original comment by dclinton on 29 Sep 2008 at 4:55

[GoogleCodeExporter]

that sounds great.  the above patch would work as described with minor changes. 
Would you also like to also pass since_id directly whenever possible (ie, issue 
22)?

I'll gladly update the patch... although you may need to rewrite the 
documentation
(since I'm not sure how that works)

Original comment by odio....@gmail.com on 29 Sep 2008 at 3:10

[GoogleCodeExporter]

Cool.  I feel like we should still try and put every parameter we know about 
(like
since_id) into the signature of each API call, and rely on extra_parameters 
only for
those that we haven't yet exposed explicitly.

Because we could go either way on this, lets say that the extra_parameters dict
overrides the explicit parameters.  I.e., extra_parameters is overlaid on top 
of the
parameters dict that was initialized via the explicit parameters.

Original comment by dclinton on 29 Sep 2008 at 3:37

[GoogleCodeExporter]

Ok sounds good.  I'll update the patch.

If it's more elegant to make the parameters dict override extra_parameters - 
would
you prefer that path?

Original comment by odio....@gmail.com on 30 Sep 2008 at 6:18

[GoogleCodeExporter]

I'm not sure which is more intuitive for users.  What do you think?  We should
document it clearly no matter what.

Code wise it should be the same either way, either initializing the parameter 
dict
with extra_parameters first, or updating it at the end.

Original comment by dclinton on 30 Sep 2008 at 6:24

[GoogleCodeExporter]

Personally, I feel like if you're explicitly passing the method a parameter it 
should
override what's in the dictionary.

Also, this would allow for the following code:

def GetPublicTimeline(self, extra_parameters={}, since_id=None):
  if since_id:
    extra_parameters['since_id'] = since_id
  url = 'http://twitter.com/statuses/public_timeline.json'
  json = self._FetchUrl(url,  parameters=extra_parameters)
  data = simplejson.loads(json)
  return [Status.NewFromJsonDict(x) for x in data]

Original comment by odio....@gmail.com on 30 Sep 2008 at 6:33

[GoogleCodeExporter]

I'm okay with the decision to use extra_parameters as the initial values.

However, for the sake of protecting the mutable parameter, please consider 
writing
that as:

def GetPublicTimeline(self, since_id=None, extra_parameters=None):
  if extra_parameters:
    parameters = dict(extract_parameters)
  else:
    parameters = {}
  if since_id:
    parameters['since_id'] = since_id
  url = 'http://twitter.com/statuses/public_timeline.json'
  json = self._FetchUrl(url,  parameters=parameters)
  # ...

Original comment by dclinton on 30 Sep 2008 at 6:54

[GoogleCodeExporter]

Ok.. what about:

def GetPublicTimeline(self, since_id=None, extra_parameters={}):
  parameters = extra_parameters
  if since_id:
    parameters['since_id'] = since_id
  ...

Original comment by odio....@gmail.com on 30 Sep 2008 at 7:03

[GoogleCodeExporter]

No, that would still leave the argument open to modification, which is what the
defensive copy prevents.

In fact, there are two bugs there.

Here's the first bug:

  >>> my_params = {'since_id': 123}
  >>> my_params['since_id']
  123
  >>> api.GetPublicTimeline(since_id = 456, extra_parameters=my_params)
  >>> my_params['since_id']
  456

Note that calling GetPublicTimeline modified the dict that was passed in.  Not 
good.

Generally speaking you should always make defensive copies of mutable 
parameters, but
especially in the case where you know you're going to be modifying them.

But the other bug is even worse.  The "extra_parameters={}" in the parameter 
list is
the problem, as the exact same default dict will be reused between calls, and 
it will
be modified each time.

For example:

  >>> def danger(d={}, a=None):
  ...   if a:
  ...     d['a'] = a  # bug! if d wasn't passed in explicitly, the default {} is
reused and modified over and over
  ...   print d
  ... 
  >>> danger()
  {}
  >>> danger(a=2)
  {'a': 2}
  >>> danger()
  {'a': 2}  # whoa!  how'd that happen?

Scary, right?

The way to prevent that is to say:

  >>> def safe(d=None, a=None):
  ...   if d is None:
  ...     d = {}  # safe, because it initializes a new empty dict each time
  ...   if a:
  ...     d['a'] = a
  ...   print d
  ... 
  >>> safe()
  {}
  >>> safe(a=2)
  {'a': 2}
  >>> safe()
  {}

Make sense?

-DeWitt

Original comment by dclinton on 30 Sep 2008 at 7:30

[GoogleCodeExporter]

OK - makes sense.  Sorry about that, I'm still learning python :)

Original comment by odio....@gmail.com on 30 Sep 2008 at 8:11

[GoogleCodeExporter]

No problem at all!  Happy to help teach it.

Original comment by dclinton on 30 Sep 2008 at 8:17

[GoogleCodeExporter]

Probably a month too late, but have you considered using keyword arguments for 
the
extra parameters? They pack up the arguments into a new dictionary 
automatically.

  def GetPublicTimeline(self, **extra_parameters):
    url = 'http://twitter.com/statuses/public_timeline.json'
    json = self._FetchUrl(url, extra_parameters)
    data = simplejson.loads(json)
    return [Status.NewFromJsonDict(x) for x in data]

And you can pass them in like you do ordinary parameters:

  api.GetPublicTimeline(since_id=123, count=42, etc=1)

No need to make copies of dictionaries, or create temporary ones, or worry about
which 'since_id' parameter takes precedence. 

Original comment by kjd...@gmail.com on 31 Oct 2008 at 7:43

[GoogleCodeExporter]

Original comment by dclinton on 26 Apr 2009 at 5:56

• Changed state: Accepted