Compatibility with RPGGeek

[edjw]

I'm having some issues getting data of out RPGGeek with this. I can't use game(), search(), or get_game_id() for RPGs. It looks like searching for boardgames is hardcoded in.

[lcosmin]

Yeah, the library doesn't support RPGGeek/VideoGameGeek ☹️ and I'm not sure it's only a matter of removing the hardcoded stuff (the API responses might have a slightly different structure for rpg/videogames).

I'll look into it when I get a chance, but if you need this soon you can try to remove what's hardcoded and see what happens. Pull requests welcome. 😃

[pak21]

I had a quick look at this. It's fundamentally not hard - adding rpgitem to the list of allowed types doesn't crash and gives you an object with at least some of the properties (e.g. title) filled in correctly. Other properties won't be too hard as they're just renamed with the type - e.g. boardgamepublisher becomes videogamepublisher. The API could probably do with some thought though:

• Does it make sense to call BGGClient().game(game_id=68407) to get information about Super Mario Bros.? Doesn't seem entirely silly as Super Mario Bros. is a game.
• Does it make sense to call BGGClient().game(game_id=43359) to get information about the AD&D 1e Players Handbook? Not really as the Players Handbook isn't a game, it's a book - the game itself is AD&D 1e which has its own entry on RPGGeek.
• Does it make sense for either of those calls to return a BoardGame object? No, they're not board games!

If we're not worried about this inconsistencies I'll make a start on this - I imagine the code will be somewhat reusable whatever we do.

[pak21]

OK, here's a very quick hack to enable support for video games and RPG items. Works and gets data but would really need some thought as to object types and which properties each type should have if we take it any further - e.g. while all of board games, video games and RPG items have a publisher, only board games have categories, only video games have genres etc.

[lcosmin]

I started to work on this a while ago, but I haven't made much progress. I believe I was trying to organize the API around the concept of "thing" instead of "game", to avoid the issue you're raising (i.e. a player's handbook not being a game). I'll take a look at my branch and push it (today or tomorrow) if there's something worth salvaging, maybe you can take a look at it, as I don't have much time to work on this.

[pak21]

That's the route I was thinking of going down as well: have a BGGClient.thing(thing_id=12345) call which could return any of an BoardGame, VideoGame or RPGItem object depending on what the id represents. I haven't dug deep enough to work out what class hierarchy should exist for those objects yet.

(And for backwards compatibility reasons, make BGGClient.game(game_id=12345) be an alias for BGGClient.thing(thing_id=12345)).

[lcosmin]

I pushed feature/support-rpggeek with the code I had. If you want to give it a shot and implement something supporting RPGGeek too, please do :)

[pak21]

OK, I've done some more work on this. It's not ready to "go live" yet but let me know your thoughts. Some implementation notes:

• BGGClient.game() can now return one of three object types BoardGame, VideoGame or RPGItem. These have only the properties relevant for that type e.g. no genre on board games or designer on video games.
• Yes, there is multiple inheritance. I know it can be a hot topic but I think it makes sense here where is a "matrix" of property combinations depending on whether it's a full item from the thing API vs the abbreviated item from the collection API and on board game vs video game vs RPG item.
• At the moment, the VideoGame and RPGItem classes rely on the __getattr__ property of DictObject to expose their properties. This does mean a lack of tedious boilerplate code, but also makes it harder to discover those properties. My personal take is that I don't think we should have both __getattr__ and explicit properties so we should drop one behaviour or the other, probably __getattr__ because that's confusing for consumers of the API.
• I haven't yet made any changes to the API so BGGClient.game() would still need to be changed to BGGClient.thing() like you've done in your branch.

If you get a chance to look at it, any comments are appreciated!

[lcosmin]

* `BGGClient.game()` can now return one of three object types `BoardGame`, `VideoGame` or `RPGItem`. These have only the properties relevant for that type e.g. no genre on board games or designer on video games.

Ugh, can't help feeling that the BGG API is a mess 😞 When I started working on this, I was under the (wrong) impression that you must use www.boardgamegeek.com/xmlapi2 for boardgames, www.rpggeek.com/xmlapi2 for rpgs, etc., which is the reason I decided to have a BGGCommon class which implemented the common functionality for all sites, BGGClient was the client for boardgames, RPGClient should have been the one for rpg stuff and VGClient for videogames. But now I see it's only a matter of the id used and everything works with every entry point. Some refactoring might be needed, as BGGCommon could be merged with BGGClient now.

Back to your point: given my revelation above, I think it's ok to use BGGClient for everything and not implement a RPGClient/VGClient too.
However, I always assumed that .game() would be dealing with boardgames only, so I suggest having a .rpg() and a .videogame() which internally would call another public method, .thing() which will contain all the logic and return every object type (BoardGame, VideoGame, RPGItem).

* Yes, there is multiple inheritance. I know it can be a hot topic but I think it makes sense here where is a "matrix" of property combinations depending on whether it's a full item from the `thing` API vs the abbreviated item from the `collection` API and on board game vs video game vs RPG item.

I've nothing against multiple inheritance if it makes sense, but looking at what's going on there I see that it's starting to get messy: for example, FullItem is used by every "final" object (BoardGame, VideoGame and RPGItem) but it has methods specific to boardgames only: add_comment(), which adds a BoardGameComment() and add_expanded_game() which doesn't make sense for RPGItems (or does it? Can a rpg item expand a game?)

* At the moment, the `VideoGame` and `RPGItem` classes rely on the `__getattr__` property of `DictObject` to expose their properties. This does mean a lack of tedious boilerplate code, but also makes it harder to discover those properties. My personal take is that I don't think we should have _both_ `__getattr__` and explicit properties so we should drop one behaviour or the other, probably `__getattr__` because that's confusing for consumers of the API.

Most objects use @property to help with documentation as well as allowing IDEs to autocomplete things, where possible, so I'd go with this approach even if it's verbose.

* I haven't yet made any changes to the API so `BGGClient.game()` would still need to be changed to `BGGClient.thing()` like you've done in your branch.

If you get a chance to look at it, any comments are appreciated!

I think you've made good progress, thanks! If you want to continue, here's a few of the things I'd implement/change:

• .rpg(), .videogame(), .thing() on BGGClient
• create_game_from_xml should be renamed to something more apropriate (create_thing_from_xml ? although it might be confusing because of my Thing class :( ) and it should be reexamined to see if all those operations make sense for every type of item (I mean looking for boardgameexpansions, boardgameacessories, videos, etc.).
• Merge BGGCommon with BGGClient and remove any code that's not needed anymore
• Rethink the class hierarchy, see if it can be simplified