search

Eigenbahn / ai-dungeon-cli

:european_castle: A cli client to play.aidungeon.io
MIT License
150 stars 34 forks source link

OOP rewrite of conf and user I/O code #20

Closed p3r7 closed 4 years ago

p3r7 commented 4 years ago

Here is a proposal for a continuation of the work made in https://github.com/Eigenbahn/ai-dungeon-cli/pull/4.

@idangur, @otreblan and @Zer0xFF I would really appreciate if you could do a small code review.

tl;dr

Details

I wasn't super satisfied w/ having the config loading being done in the game logic class AiDungeon (bad separation of concerns).

Also, the coding style was heterogeneous between the user / terminal I/O code (module as an object) and the rest (objects are class). So I also turned it into class(es).

It should (I hope) help greatly using this package as a library to integrate it with other systems (like @Zer0xFF did in https://github.com/Eigenbahn/ai-dungeon-cli/pull/9).

I could have gone full retard and extract API calls from class AiDungeon into a dedicated one as well but as there is only one public instance of this API it's not worth the effort.

p3r7 commented 4 years ago

Oopsie, well spotted. Should be fixed.

Zer0xFF commented 4 years ago

from a visual inspection it looks good, but just a nitpick comment. config file path should be an argument, with perhaps defualt value.

p3r7 commented 4 years ago

@Zer0xFF I thought about adding argparse to be able to change behavior via arguments.

All config keys would be mapped to parameters, making the config file either optional or fallback values.

But I didn't think about having an argument to load a file from a specific location. I'm taking note.