GNOME Bugzilla – Bug 777980
Add a public wrapper for GDataParsable:get_content_type
Last modified: 2017-02-08 16:47:56 UTC
In bug 771825 and bug 771823, we are trying to marshal a GDataEntry (specifically a GDataPicasaWebFile at the moment) across D-Bus. We can hardcode the content-type, or poke at the GDataParsableClass, but it would be nice if we can avoid that. Something like gdata_parsable_get_content_type would help.
Created attachment 344648 [details] [review] core: Add a public wrapper for GDataParsable:get_content_type
Review of attachment 344648 [details] [review]: Would be good to add a few calls to this into the tests. How about the test_entry_[get|parse]_[xml|json]() tests in gdata/tests/general.c? ::: gdata/gdata-parsable.c @@ +552,3 @@ + * @self: a #GDataParsable + * + * Returns the content type upon which the #GDataParsable is built. For example, application/atom+xml or application/json. Nitpick: Put backticks around the two example content types: `application/atom+xml` or `application/json`. This is the gtk-doc/Markdown formatting for monospaced text.
Created attachment 344658 [details] [review] core: Add a public wrapper for GDataParsable:get_content_type
(In reply to Philip Withnall from comment #2) > Review of attachment 344648 [details] [review] [review]: > > Would be good to add a few calls to this into the tests. How about the > test_entry_[get|parse]_[xml|json]() tests in gdata/tests/general.c? Base classes like GDataEntry and GDataFeed support both XML and JSON, and their content-type is almost non-consequential. So, I only added assertions for those types that implement either the XML or JSON virtual methods. I guess the real value would be to assert it for more leaf-level or service-specific classes where the content-type really matters. By the way, what's up with GDataAPPCategories? It only overrides the JSON parsing vfuncs, but doesn't override the get_content_type. Does it also support both XML and JSON?
Created attachment 344661 [details] [review] tests: Assert the content-type of various GDataParsable types A quick first pass at asserting the content-type.
Review of attachment 344658 [details] [review]: Looks good, thanks.
(In reply to Debarshi Ray from comment #4) > (In reply to Philip Withnall from comment #2) > > Review of attachment 344648 [details] [review] [review] [review]: > > > > Would be good to add a few calls to this into the tests. How about the > > test_entry_[get|parse]_[xml|json]() tests in gdata/tests/general.c? > > Base classes like GDataEntry and GDataFeed support both XML and JSON, and > their content-type is almost non-consequential. So, I only added assertions > for those types that implement either the XML or JSON virtual methods. > > I guess the real value would be to assert it for more leaf-level or > service-specific classes where the content-type really matters. Yeah, but that would be a separate set of tests: to ensure that the leaf implementations are setting the *correct* content-types. The tests I wanted here (and which you have added, thanks) are just to check that the get_content_type() method doesn’t explode. > By the way, what's up with GDataAPPCategories? It only overrides the JSON > parsing vfuncs, but doesn't override the get_content_type. Does it also > support both XML and JSON? Whoops, that’s an omission. Could you whip up a patch to add the missing get_content_type() vfunc implementation? GDataAPPCategories only supports JSON — it was ported from XML to JSON for the latest version of the YouTube API a while ago.
Review of attachment 344661 [details] [review]: Looks good apart from the FIXME, which can be removed after GDataAPPCategories is fixed.
Comment on attachment 344658 [details] [review] core: Add a public wrapper for GDataParsable:get_content_type Pushed to master.
> Whoops, that’s an omission. Could you whip up a patch to add the missing > get_content_type() vfunc implementation? GDataAPPCategories only supports > JSON — it was ported from XML to JSON for the latest version of the YouTube > API a while ago. Ok, I will do that.
Created attachment 345035 [details] [review] app: Specify the correct content type
Created attachment 345036 [details] [review] tests: Assert the content-type of various GDataParsable types
Review of attachment 345035 [details] [review]: Yes!
Review of attachment 345036 [details] [review]: Looks good, thanks.
Thanks for the reviews! Pushed to master.