search

lcosmin / boardgamegeek

A Python interface to boardgamegeek.com. Pulls information from BGG and creates Python objects for the data.
BSD 3-Clause "New" or "Revised" License
117 stars 71 forks source link

Compatibility with RPGGeek #43

Open edjw opened 6 years ago

edjw commented 6 years ago

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 commented 6 years ago

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 commented 5 years ago

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:

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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:

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

lcosmin commented 5 years ago 
* `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: