Add support for custom query parameters.

[Ali Afshar, 2011-03-31 07:16:26]

Hi Alain, I wasn't on the review list, but I hope you don't mind.

http://codereview.appspot.com/4326045/diff/1/src/gdata/client.py
File src/gdata/client.py (right):

http://codereview.appspot.com/4326045/diff/1/src/gdata/client.py#newcode815
src/gdata/client.py:815: kwargs: other query parameters that are not explicitly
defined.
Perhaps use a more descriptive name here, such as default_query_parameters (if
that is not too long).

http://codereview.appspot.com/4326045/diff/1/src/gdata/client.py#newcode829
src/gdata/client.py:829: self.kwargs = kwargs
Consider setting to the empty dict if the parameter is not passed, so it will
not have to be checked later in modify_request.

http://codereview.appspot.com/4326045/diff/1/src/gdata/client.py#newcode855
src/gdata/client.py:855: for key in self.kwargs:
Use dict.update, eg: http_requst.uri.query.update(self.default_query_parameters)

[alainv, 2011-03-31 16:31:29]

Hey Ali,

Thanks for the reviews! I applied what you requested, but I was also wondering
if it was a good thing to add the custom_parameters to the constructor or if we
should just add the "add_custom_parameter" method.
What do you think?

Best,
Alain

http://codereview.appspot.com/4326045/diff/1/src/gdata/client.py
File src/gdata/client.py (right):

http://codereview.appspot.com/4326045/diff/1/src/gdata/client.py#newcode815
src/gdata/client.py:815: kwargs: other query parameters that are not explicitly
defined.
On 2011/03/31 07:16:26, Ali Afshar wrote:
> Perhaps use a more descriptive name here, such as default_query_parameters (if
> that is not too long).

Done.

http://codereview.appspot.com/4326045/diff/1/src/gdata/client.py#newcode829
src/gdata/client.py:829: self.kwargs = kwargs
On 2011/03/31 07:16:26, Ali Afshar wrote:
> Consider setting to the empty dict if the parameter is not passed, so it will
> not have to be checked later in modify_request.

Done.

http://codereview.appspot.com/4326045/diff/1/src/gdata/client.py#newcode855
src/gdata/client.py:855: for key in self.kwargs:
On 2011/03/31 07:16:26, Ali Afshar wrote:
> Use dict.update, eg:
http_requst.uri.query.update(self.default_query_parameters)

Done.

[Ali Afshar, 2011-03-31 17:00:42]

LGTM

And yes, you could leave out the method, and custom_parameters could be part of
the "public" interface. So if someone really wanted to add one (and I assume it
would be strange cases only), they would just use it. (Also LGTM)

On 2011/03/31 16:31:29, alainv wrote:
> Hey Ali,
> 
> Thanks for the reviews! I applied what you requested, but I was also wondering
> if it was a good thing to add the custom_parameters to the constructor or if
we
> should just add the "add_custom_parameter" method.
> What do you think?
> 
> Best,
> Alain
> 
> http://codereview.appspot.com/4326045/diff/1/src/gdata/client.py
> File src/gdata/client.py (right):
> 
> http://codereview.appspot.com/4326045/diff/1/src/gdata/client.py#newcode815
> src/gdata/client.py:815: kwargs: other query parameters that are not
explicitly
> defined.
> On 2011/03/31 07:16:26, Ali Afshar wrote:
> > Perhaps use a more descriptive name here, such as default_query_parameters
(if
> > that is not too long).
> 
> Done.
> 
> http://codereview.appspot.com/4326045/diff/1/src/gdata/client.py#newcode829
> src/gdata/client.py:829: self.kwargs = kwargs
> On 2011/03/31 07:16:26, Ali Afshar wrote:
> > Consider setting to the empty dict if the parameter is not passed, so it
will
> > not have to be checked later in modify_request.
> 
> Done.
> 
> http://codereview.appspot.com/4326045/diff/1/src/gdata/client.py#newcode855
> src/gdata/client.py:855: for key in self.kwargs:
> On 2011/03/31 07:16:26, Ali Afshar wrote:
> > Use dict.update, eg:
> http_requst.uri.query.update(self.default_query_parameters)
> 
> Done.

[Ali Afshar, 2011-03-31 17:02:04]

Sorry to re-review, but I added a comment.

http://codereview.appspot.com/4326045/diff/4001/src/gdata/client.py
File src/gdata/client.py (right):

http://codereview.appspot.com/4326045/diff/4001/src/gdata/client.py#newcode769
src/gdata/client.py:769: max_results=None, strict=False, **custom_parameters):
Should this be kwargs or a dict, it's not clear from the rest of the
implementation.

[alainv, 2011-03-31 17:05:18]

http://codereview.appspot.com/4326045/diff/4001/src/gdata/client.py
File src/gdata/client.py (right):

http://codereview.appspot.com/4326045/diff/4001/src/gdata/client.py#newcode769
src/gdata/client.py:769: max_results=None, strict=False, **custom_parameters):
On 2011/03/31 17:02:04, Ali Afshar wrote:
> Should this be kwargs or a dict, it's not clear from the rest of the
> implementation.

This should be a kwargs so you can construct a Query like this:
   query = gdata.data.Query(myCustomParam='someValue', ...)

But as I said earlier, it might be better to remove this parameter and just
leave the public "add_custom_parameter" method so that users can add those
parameters manually without being confused about how to use the constructor.

[Ali Afshar, 2011-03-31 17:10:02]

I can see justification for both, so it's up to you.

Personally I would leave it how it is in the constructor, but then you don't
need the "= blah or {}" because it will default to a new empty dict.

On 2011/03/31 17:05:18, alainv wrote:
> http://codereview.appspot.com/4326045/diff/4001/src/gdata/client.py
> File src/gdata/client.py (right):
> 
> http://codereview.appspot.com/4326045/diff/4001/src/gdata/client.py#newcode769
> src/gdata/client.py:769: max_results=None, strict=False, **custom_parameters):
> On 2011/03/31 17:02:04, Ali Afshar wrote:
> > Should this be kwargs or a dict, it's not clear from the rest of the
> > implementation.
> 
> This should be a kwargs so you can construct a Query like this:
>    query = gdata.data.Query(myCustomParam='someValue', ...)
> 
> But as I said earlier, it might be better to remove this parameter and just
> leave the public "add_custom_parameter" method so that users can add those
> parameters manually without being confused about how to use the constructor.