Issue 4444060: Add support for Protocol Buffers as an API serialization format
| Created: 14 years, 6 months ago by mmcdonald_google Modified: 14 years, 6 months ago Reviewers: jcgregorio_google CC: google-api-python-client_googlegroups.com Visibility: Public. | Patch Set 1 #Patch Set 2 : additional docstrings # Total comments: 2 Patch Set 3 : "204 No Content" response tests #Patch Set 4 : Remove LoggingJsonModel #Patch Set 5 : Fix samples/debugging/main.py #
MessagesTotal messages: 9
http://codereview.appspot.com/4444060/diff/2001/apiclient/model.py File apiclient/model.py (right): http://codereview.appspot.com/4444060/diff/2001/apiclient/model.py#newcode302 apiclient/model.py:302: Please add a LoggingProtocolBufferModel also. http://codereview.appspot.com/4444060/diff/2001/tests/test_protobuf_model.py File tests/test_protobuf_model.py (right): http://codereview.appspot.com/4444060/diff/2001/tests/test_protobuf_model.py#... tests/test_protobuf_model.py:96: def test_good_response_wo_data(self): Add a test case for 204 also. And also a 204 test case to the json model tests. Sign in to reply to this message.
On 2011/04/27 01:33:15, jcgregorio_google wrote: > http://codereview.appspot.com/4444060/diff/2001/apiclient/model.py > File apiclient/model.py (right): > > http://codereview.appspot.com/4444060/diff/2001/apiclient/model.py#newcode302 > apiclient/model.py:302: > Please add a LoggingProtocolBufferModel also. As I look with an eye to limit duplicated code while doing this, I'm curious about the choice of even having this logging functionality in a subclass. Why not just have this be part of the parent BaseModel so they can both just inherit it? > http://codereview.appspot.com/4444060/diff/2001/tests/test_protobuf_model.py > File tests/test_protobuf_model.py (right): > > http://codereview.appspot.com/4444060/diff/2001/tests/test_protobuf_model.py#... > tests/test_protobuf_model.py:96: def test_good_response_wo_data(self): > Add a test case for 204 also. And also a 204 test case to the json model tests. Done. Sign in to reply to this message.
On 2011/04/27 20:59:15, mmcdonald wrote: > On 2011/04/27 01:33:15, jcgregorio_google wrote: > > http://codereview.appspot.com/4444060/diff/2001/apiclient/model.py > > File apiclient/model.py (right): > > > > http://codereview.appspot.com/4444060/diff/2001/apiclient/model.py#newcode302 > > apiclient/model.py:302: > > Please add a LoggingProtocolBufferModel also. > > As I look with an eye to limit duplicated code while doing this, I'm curious > about the choice of even having this logging functionality in a subclass. Why > not just have this be part of the parent BaseModel so they can both just inherit > it? SGTM, let's do that. > > > > http://codereview.appspot.com/4444060/diff/2001/tests/test_protobuf_model.py > > File tests/test_protobuf_model.py (right): > > > > > http://codereview.appspot.com/4444060/diff/2001/tests/test_protobuf_model.py#... > > tests/test_protobuf_model.py:96: def test_good_response_wo_data(self): > > Add a test case for 204 also. And also a 204 test case to the json model > tests. > > Done. Sign in to reply to this message.
On Wed, Apr 27, 2011 at 5:08 PM, <jcgregorio@google.com> wrote: > On 2011/04/27 20:59:15, mmcdonald wrote: > >> On 2011/04/27 01:33:15, jcgregorio_google wrote: >> > http://codereview.appspot.com/4444060/diff/2001/apiclient/model.py >> > File apiclient/model.py (right): >> > >> > >> > > http://codereview.appspot.com/4444060/diff/2001/apiclient/model.py#newcode302 > >> > apiclient/model.py:302: >> > Please add a LoggingProtocolBufferModel also. >> > > As I look with an eye to limit duplicated code while doing this, I'm >> > curious > >> about the choice of even having this logging functionality in a >> > subclass. Why > >> not just have this be part of the parent BaseModel so they can both >> > just inherit > >> it? >> > > SGTM, let's do that. To clarify, do you stilll want a separate Logging subclass for each, or just let JsonModel and ProtocolBufferModel output log info directly if given the proper flags (and remove the LoggingJsonModel class)? Sign in to reply to this message.
On Wed, Apr 27, 2011 at 5:21 PM, Matt McDonald <mmcdonald@google.com> wrote: > On Wed, Apr 27, 2011 at 5:08 PM, <jcgregorio@google.com> wrote: >> >> On 2011/04/27 20:59:15, mmcdonald wrote: >>> >>> On 2011/04/27 01:33:15, jcgregorio_google wrote: >>> > http://codereview.appspot.com/4444060/diff/2001/apiclient/model.py >>> > File apiclient/model.py (right): >>> > >>> > >> >> >> http://codereview.appspot.com/4444060/diff/2001/apiclient/model.py#newcode302 >>> >>> > apiclient/model.py:302: >>> > Please add a LoggingProtocolBufferModel also. >> >>> As I look with an eye to limit duplicated code while doing this, I'm >> >> curious >>> >>> about the choice of even having this logging functionality in a >> >> subclass. Why >>> >>> not just have this be part of the parent BaseModel so they can both >> >> just inherit >>> >>> it? >> >> SGTM, let's do that. > > To clarify, do you stilll want a separate Logging subclass for each, or just > let JsonModel and ProtocolBufferModel output log info directly if given the > proper flags (and remove the LoggingJsonModel class)? Let's have JsonModel and ProtocolBufferModel log that info directly based on the flag, and remove LoggingJsonModel. Sign in to reply to this message.
On Wed, Apr 27, 2011 at 5:24 PM, Joe Gregorio <jcgregorio@google.com> wrote: > Let's have JsonModel and ProtocolBufferModel log that info directly > based on the flag, > and remove LoggingJsonModel > Done. Sign in to reply to this message.
Looking good. Please update samples/debugging/main.py to no longer use LoggingJsonModel. On 2011/04/27 21:33:37, mmcdonald wrote: > On Wed, Apr 27, 2011 at 5:24 PM, Joe Gregorio <mailto:jcgregorio@google.com> wrote: > > > Let's have JsonModel and ProtocolBufferModel log that info directly > > based on the flag, > > and remove LoggingJsonModel > > > > Done. Sign in to reply to this message.
On 2011/04/29 14:42:48, jcgregorio_google wrote: > Looking good. Please update samples/debugging/main.py to no > longer use LoggingJsonModel. Done. Sign in to reply to this message. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
