Turned into a package + made more Pythonic

Eigenbahn / ai-dungeon-cli

:european_castle: A cli client to play.aidungeon.io

MIT License

150 stars 34 forks source link

Turned into a package + made more Pythonic #4

Closed idangur closed 4 years ago

idangur commented 4 years ago

Changed the code to be more pythonic, using classes and raising self made exceptions, for a cleaner and better ordered code, will continue to be adding features to this PR, as i really liked what you did with this idea and I love Ai Dungeon!

otreblan commented 4 years ago

Now it is on the aur

p3r7 commented 4 years ago

Wow, between the 2 (or is it 3?) of us we've got a real 24/7 operation running :astonished:.

I had the thought of doing a proper python package, but you actually did the thing!

A few remarks:

[x] Please resolve the conflicts. I added the possibility to have credentials as a login config + made the UTF8 splash logo 80 cols. They should be easy to integrate.
[x] The PR name focused of form ("we did that") over function ("we implemented that"). Thus I renamed it.
[x] Let's not embark too many stuff in a single PR / branch. I see that your branch name is create-user-if-not-configured. Let's us please first finish and merge the packaging side of things and then create another PR. Otherwise we'll ultimately fail to let others contribute and maintain a stable main repo.

For now I'll stop doing stuff on the main repo, waiting for this to be merged.

idangur commented 4 years ago

Ill add the changes you made today, as soon as ill get to my pc, and then i think it willbe ok to merge

idangur commented 4 years ago

Finished with the changes for now, I think its ready for a merge, would be glad if you can review!

idangur commented 4 years ago

fixed it

p3r7 commented 4 years ago

Could you also mark the conflicts as resolved? This will facilitate merging.

I'll merge this evening or tomorrow morning (CEST).

I've already created a release on the current branch to signify the following drastic change (of the code structure).

otreblan commented 4 years ago

https://github.com/idangur/ai-dungeon-cli/pull/4

idangur commented 4 years ago

all conflicts resolved, thanks orteblan!

p3r7 commented 4 years ago

A bug fix w/ login() + updated README + various minor refactoring.

https://github.com/idangur/ai-dungeon-cli/pull/5

p3r7 commented 4 years ago

Ta-dam!

Thanks fro the good work @idangur and @otreblan.

Some comments about the last commits.

global term vars

Fine by me. Their not really part of the game logic. So it's better if they're separate (global or made into a class if they start to go up in number).

Furthermore, I thought already of maybe doing a systematic lookup of terminal width, to handle the fact that people tend to resize their term emulator windows.

handling of /quit

Also, maybe you didn't see my last comment in https://github.com/idangur/ai-dungeon-cli/pull/5. I proposed raising a QuitSession instead of relying on stop_session.

What do you think about it?

idangur commented 4 years ago

You can raise QuitSession, if you think it fits accordingly, just remember to change the while loop to while true, even though I think it's a bit of bad practice (exiting a loop using an error)

p3r7 commented 4 years ago

it's a bit of bad practice (exiting a loop using an error)

Yes indeed, ideally exceptions should be caught as early as possible and translated to "clean" state logic.

But we're already allowing to exit the loop with the KeyboardInterrupt and ConnectionError exceptions. Even tough those are a standard exceptions, the effect is the same.

But I guess that's OK given the low complexity of the code (as of now).

@idangur, are you ok with this reasoning?