GNOME Bugzilla – Bug 766678
Add TheGamesDB source
Last modified: 2016-05-28 22:57:18 UTC
.
Created attachment 328219 [details] [review] tests: Rename gamefaqs tests to games As we want to use this test suite for all games.
Created attachment 328220 [details] [review] tests: Fix dist with lua-factory disabled
Created attachment 328221 [details] [review] lua-factory: Add TheGamesDB source
Created attachment 328222 [details] [review] tests: Port games test from gamesfaq to thegamesdb
Created attachment 328223 [details] [review] lua-factory: Remove GameFAQs source The website managers made it clear that they did not want anyone to use the search engine to find metadata, as stated in the robots.txt file. As this won't be useful to gnome-games, remove it.
Created attachment 328226 [details] [review] tests: Port games test from gamesfaq to thegamesdb
Created attachment 328250 [details] [review] tests: Add test cases to thegamesdb
Review of attachment 328250 [details] [review]: attachment 328226 [details] [review] already has a port of the tests.
Created attachment 328256 [details] [review] test: Add two test cases to thegamesdb Add thumbnail tests for: - Kirby's Dream Land on Game boy - Sonic th Hedgehog on Master System
Review of attachment 328256 [details] [review]: ::: tests/games/data/resolve-getgame-kirby-gb.data @@ +4,3 @@ +<Game> +<id>3190</id> +<GameTitle>Kirby's Dream Land 2</GameTitle> That's not a GetGame output, that's a GetGamesList output. ::: tests/games/data/resolve-getgame-sonic-sms.data @@ +4,3 @@ +<Game> +<id>3014</id> +<GameTitle>Sonic the Hedgehog Chaos</GameTitle> Same thing here. ::: tests/games/data/resolve-kirby-gb.data @@ +2,3 @@ + +<Data> +<baseImgUrl>http://thegamesdb.net/banners/</baseImgUrl> *This* is the GetGame output and not the GetGamesList output. ::: tests/games/data/resolve-sonic-sms.data @@ +2,3 @@ + +<Data> +<baseImgUrl>http://thegamesdb.net/banners/</baseImgUrl> Ditto. ::: tests/games/test_games.c @@ +168,3 @@ + test_resolve_thumbnail_found (source, keys, options, + "Kirby's Dream Land", + "application/x-game-boy-rom", that's application/x-gameboy-rom, not application/x-game-boy-rom
Created attachment 328290 [details] [review] lua-factory: Add TheGamesDB source
Created attachment 328291 [details] [review] test: Add two test cases to thegamesdb Add thumbnail tests for: - Kirby's Dream Land on Game boy - Sonic th Hedgehog on Master System
(In reply to Bastien Nocera from comment #11) > Created attachment 328290 [details] [review] [review] > lua-factory: Add TheGamesDB source Correct the disambiguation that still used numerical IDs for platforms (inherited from the gamefaqs script), and now goes through an exact match test before returning a result. (In reply to Bastien Nocera from comment #12) > Created attachment 328291 [details] [review] [review] > test: Add two test cases to thegamesdb > > Add thumbnail tests for: > - Kirby's Dream Land on Game boy > - Sonic th Hedgehog on Master System I corrected the test data, and the mime-type so that we didn't try to find a Kirby's Dreamland on Neo Geo or something ;)
Review of attachment 328219 [details] [review]: looks good
Review of attachment 328220 [details] [review]: sure
Review of attachment 328223 [details] [review]: pity :(
Review of attachment 328226 [details] [review]: sure
Review of attachment 328290 [details] [review]: Small description about the source in commit log? Patch looks good. Could you please rebase it? I was not able to apply cleanly and I'd like to try it out before acking ::: src/lua-factory/sources/grl-thegamesdb.lua @@ +49,3 @@ +--------------------------------- + +function get_platform_name(mime_type, suffix) This function could be moved under Utilities sector instead of Handlers sector @@ +52,3 @@ + local platform_names = {} + + -- mime-types are upstream in shared-mime-info Where? Can't we load this and the names as GResource? @@ +112,3 @@ + if not url then return nil end + -- Return a 3-letter suffix + return url:match('.-%.(...)$') I think you want to use the wildcard '*' instead of '-' otherwise it could fail to get the suffix if uri has dots in the name grl-video-title-parsing also gets the suffix (but remove it) https://git.gnome.org/browse/grilo-plugins/tree/src/lua-factory/sources/grl-video-title-parsing.lua#n64 @@ +240,3 @@ + + if game.Developer then + -- FIXME media.developer = game.Developer.xml You would rather that we include this metadata in grilo or that the lua source iteself could create them like the c plugins do? https://bugzilla.gnome.org/show_bug.cgi?id=762567
Review of attachment 328291 [details] [review]: Looks good ::: tests/games/test_games.c @@ +176,3 @@ + "application/x-sms-rom", + 0, + "http://thegamesdb.net/banners/boxart/original/front/3016-1.jpg"); I would go for an array of testing-struct like [0]. That way, it is easy to include test cases. But either way is fine for me. [0] https://git.gnome.org/browse/grilo-plugins/tree/tests/thetvdb/test_thetvdb_resolve_episodes.c#n108
The attachments ended up broken making it impossible to test. Please file new bugs for any errors in those. Attachment 328219 [details] pushed as 52defab - tests: Rename gamefaqs tests to games Attachment 328220 [details] pushed as d53499d - tests: Fix dist with lua-factory disabled Attachment 328223 [details] pushed as 15841f3 - lua-factory: Remove GameFAQs source Attachment 328226 [details] pushed as 36235f0 - tests: Port games test from gamesfaq to thegamesdb Attachment 328290 [details] pushed as 8ab84cd - lua-factory: Add TheGamesDB source Attachment 328291 [details] pushed as ac18356 - test: Add two test cases to thegamesdb
(In reply to Victor Toso from comment #18) > Review of attachment 328290 [details] [review] [review]: > > Small description about the source in commit log? > > Patch looks good. Could you please rebase it? I was not able to apply > cleanly and I'd like to try it out before acking It's a problem with bugzilla or git-bz mangling the patch. It was already rebased. > ::: src/lua-factory/sources/grl-thegamesdb.lua > @@ +49,3 @@ > +--------------------------------- > + > +function get_platform_name(mime_type, suffix) > > This function could be moved under Utilities sector instead of Handlers > sector Done. > @@ +52,3 @@ > + local platform_names = {} > + > + -- mime-types are upstream in shared-mime-info > > Where? Can't we load this and the names as GResource? Those aren't names, those are identifiers used in the request. They look an awful lot like names though, I must admit. > @@ +112,3 @@ > + if not url then return nil end > + -- Return a 3-letter suffix > + return url:match('.-%.(...)$') > > I think you want to use the wildcard '*' instead of '-' otherwise it could > fail to get the suffix if uri has dots in the name > grl-video-title-parsing also gets the suffix (but remove it) > https://git.gnome.org/browse/grilo-plugins/tree/src/lua-factory/sources/grl- > video-title-parsing.lua#n64 Fixed, and fixed the two-letter suffixes too. > @@ +240,3 @@ > + > + if game.Developer then > + -- FIXME media.developer = game.Developer.xml > > You would rather that we include this metadata in grilo or that the lua > source iteself could create them like the c plugins do? > https://bugzilla.gnome.org/show_bug.cgi?id=762567 Yep, but until that's done... commit 6cffca1702e8eedd1428199feca0433a19de9e34 Author: Bastien Nocera <hadess@hadess.net> Date: Sun May 29 00:54:14 2016 +0200 thegamesdb: Move a few helpers to the helpers section See https://bugzilla.gnome.org/show_bug.cgi?id=766678#c18 commit a4ab69de82564948f6b2bb3331081690cb4b9eb9 Author: Bastien Nocera <hadess@hadess.net> Date: Sun May 29 00:51:20 2016 +0200 thegamesdb: Add support for Game Gear disambiguation We were expecting all suffixes to be 3 letters, they can be 2 in the case of the Game Gear suffix (*.gg). This also fixes the matching according to the review in: https://bugzilla.gnome.org/show_bug.cgi?id=766678#c18 commit 4176fd55a362ca132c32cbaf047687b9bed52df4 Author: Bastien Nocera <hadess@hadess.net> Date: Sun May 29 00:47:46 2016 +0200 fixup! thegamesdb: Fix cover not found when there's no back cover commit 14ecb7a588dda4fe2d0fc6b8075b2e2c7ca5c25b Author: Bastien Nocera <hadess@hadess.net> Date: Sun May 29 00:46:14 2016 +0200 tests: Add missing files to EXTRA_DIST in games commit 363c0e7727b9448cfd4f4661115c3828e9176832 Author: Bastien Nocera <hadess@hadess.net> Date: Sun May 29 00:35:02 2016 +0200 thegamesdb: Fix cover not found when there's no back cover As in the test case included ("Astérix" on the Game Gear).