Eigenbahn / ai-dungeon-cli

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

allow the setting of custom input and print handlers #9

Closed Zer0xFF closed 4 years ago

Zer0xFF commented 4 years ago

small changes to allow an external python script to have more control. if you've different suggestion as to how else to go about this, im all ears. (if its not something you'd like to merge, thats fine, as the changes are small, i wont mind perodicially updating my fork)

I've been waiting to get an ai dungeon 2 discord bot for sometime now, however running my own server (as per required spec) seemed impossible...

So i was thrilled when I found that this exist.

if you're interested, this is the bot interface for it https://github.com/Zer0xFF/DCU_Discord_Bot/blob/master/cogs/ai_dungeon.py

discord_bot.png

p3r7 commented 4 years ago

How, hey. that would be our first use-case of the package being used as a lib.

We'll sure merge this PR (though maybe with some minor adjustments).

I personally thought of an idea that could benefit from this: having two instances talk to each other, taking the output of one as the input to the other. That would make some pretty unexepected (nonsensical?) results.

Zer0xFF commented 4 years ago

haha that could be a wild trip XD

the game does make sense at times, and keep referenes back to old conversation, so it might somehow make sense.

I'll rebase it on master in a bit, but as I said, feel free to suggest any changes, and since I've already have a use case, it'd be a good test bed.

few suggestions to make this more usable as a library, remove all empty print(), and replace the other print with a. print handler.

textwrap is probably not ideal, since it creates unexpected breaks on discord, if this an issue in the terminal, the perhaps the default print handler would contain textwrap, so that when thats replaced, the dev on the otherside of the library doesnt have to deal with it.

p3r7 commented 4 years ago

Suggestion to make more usable as a lib

remove all empty print(), and replace the other print with a. print handler. textwrap is probably not ideal, since it creates unexpected breaks on discord,

Indeed.

I guess it will be better to wrap those empty print() inside default methods, as we're doing them systematically.

input_method would be set to something like:

def user_input_handler(prompt):
    user_input = input(prompt)
    print()
    return user_input

And print_method to:

def user_print_handler(text):
    print_sentences(text)
    print()
idangur commented 4 years ago

You can just add /n at the end of prints instead of an empty print method

Zer0xFF commented 4 years ago

TIL: you can just override function pointers in python

in that case we can get rid of of set methods? or keep them to make it clear that you can override those functions?

is there any point for user_print_handler() to call print_sentences()?

def user_print_handler(text):
    print("\n".join(textwrap.wrap(text, terminal_width)))
    print()
p3r7 commented 4 years ago

"/n" instead of empty print()

Yes it would work for the output part but not the input part.

For a library use-case, clients might not need / want this newline to be at the end of each and every output. Thus, wrapping it in an overridable default print method seems like the best approach.

Function pointers override VS explicit setters

Oh yeah, it didn't came to my mind (even though I've used the feature in the past to monkey-patch libs).

It's a native feature, but can seem a bit too counter-intuitive for a library use-case.

So I instinctively would prefer the setter approach.

Maybe @idangur could provide a more opinionated feedback.

Either way, usage as a lib should be documented in the README.

Necessity of print_sentences()

Yes, we can totally remove print_sentences() as it's only gets called through user_print_handler().

Name our default input / print functions

I came up with names user_input_handler() and user_print_handler() without thinking too much about them.

Something more explicit, in the line of get_user_input_term() and print_output_term(), would certainly be better.

p3r7 commented 4 years ago

Looks fine!

I will do a few tests this evening (Paris time) before merging.

p3r7 commented 4 years ago

@Zer0xFF

I've just thought about it.

https://github.com/Eigenbahn/ai-dungeon-cli/pull/19 has introduced a BC break. An additional fn story_print_handler has been introduced, that replaces some of print_handler calls.

Zer0xFF commented 4 years ago

I suppose you need to decide what you want this to be, (1) a terminal app, or (2) a library that also support termal input.

1) I know this was initially your goal, but you said you also want it to be used as a library 2) you forget about all the fancy stuff, not necessary, but you need to make sure all of that is done outside of AiDungeon, having said that, you'd ideally abstract the class down even more, instead of setting input handlers, instead you will have functions that will the required args each time. (doing so, you know exactly what each function will return, then the app can in this case for example, choose to print it out as slow print)

having said that, there are ways around this for now.

aka, you can easily fix that by making set_print_handler also call set_story_print_handler effectively resetting or anyone using the library will need to do that themselves ()

p3r7 commented 4 years ago

Thanks for your input 🍻

Yes indeed.

This project was initially a small hack I had the idea to start while waiting for my diner to end cooking.

But to enable it to grow and be used by others a "clean" design (with a lib phylosophy) is needed. If having a lib is not the primary goal, it is a necessary evil and an added bonus.

To achieve this, I'll have to sit down and think about of to restructure the code.

As mentioned, the terminal I/O code would be an input to the game logic class (AiDungeon). We could pass a set of functions as args as you suggested but could also turn all this terminal I/O code into a class.

Likewise, I'm not super fan of having the config lookup in the game logic class...

In the meantime I'll look into into your suggestion for a short term fix.