Brought code in line with PEP8, added (optional) Game World interface #8

ghost · 2018-01-03T21:51:51Z

The "Game World interface" is basically just a way to not keep facts about the world in the global scope, instead using an optional game argument to functions that (in theory) includes data about the player and world state and stuff. Also, say is now game.say, and different input/output interfaces can be used (terminal by default, but any object that inherits from adventurelib.Interface and defines obj.say(string) and obj.get_command(prompt) -> str can be used for input/output.

I also updated the demo to match, deleted unused imports, and fixed an apparent error with the fallback for getting terminal size (it was try-except-else, but that would mean that the fallback is used if shutil is present iirc; it should've been try-except(try-except)).

…, updated demo to match.

lordmauve

Overall, I'm not happy with the direction you're taking; I think you're adding accidental complexity. This is an educational library and it is deliberately written to be slightly more accessible to beginners at the expense of doing some things that are less advisable in bigger projects (eg. relying on global state).

That's not to say that there are not changes here that I think would be valuable - all the bugs you've found, for example.

Thank you for taking the time to submit a pull request.

lordmauve · 2018-01-03T22:13:21Z

adventurelib.py

 import re
 import sys
 import inspect
-import readline


Thanks -- but there changes here at once. Please make one change at a time (perhaps in different branches/pull requests).

In particular, never make style changes at the same time as functional changes. Some of your changes are bugfixes - great. I didn't realise there are so many bugs.

However, some are unnecessary. I guess your IDE is prompting you about them, but that doesn't mean it's right.

Some changes introduce bugs - and this is the real danger of doing this kind of blanket style fixing blindly.

This is the first bug - simply importing readline enables readline semantics in input(). So removing it removes those semantics.

Hm, I was not aware of that behavior with readline.

lordmauve · 2018-01-03T22:14:16Z

adventurelib.py

-    def get_terminal_size(fallback=(80, 24)):
-        return fallback
+    try:
+        from backports.shutil_get_terminal_size import get_terminal_size


Good catch, this is certainly what I intended. I don't know what I was smoking when I wrote this.

lordmauve · 2018-01-03T22:14:46Z

adventurelib.py



 __all__ = (
    'when',
    'start',
    'Room',
+    'Pattern',
+    '_handle_command',
    'Item',


Pattern and _handle_command are not part of the public API - they should not be in all.

Ah; it was used in test_adventurelib.py and my IDE complained, so I added it.

lordmauve · 2018-01-03T22:16:18Z

adventurelib.py

@@ -47,12 +53,12 @@ class Room:
    @staticmethod
    def add_direction(forward, reverse):
        """Add a direction."""
-        for dir in (forward, reverse):
-            if not dir.islower():
+        for direc in (forward, reverse):


Please don't make this change. dir is Ok if not perfect. The dir() builtin is really rarely use in real code and it certainly isn't in this function.

Most of all, please don't touch lines of code that are unrelated to your review.

Fair enough; my IDE complained and half of what I did was just trying to line the codebase up with PEP8 (I thought PEP8 said something about shadowed names, but I could be wrong)

The dir() builtin is really rarely use in real code

That's so not true. You can even make stuff easier that way and even more if you go for vars() / __dict__ e.g. with requests library -> vars(requests)[get_post_whatever_str](...) to avoid branches.

lordmauve · 2018-01-03T22:16:52Z

adventurelib.py

@@ -153,7 +160,7 @@ def __contains__(self, v):
        if isinstance(v, str):
            return bool(self.find(v))
        else:
-            return set.__contains__(v)
+            return set.__contains__(self, v)


Good catch.

It was mostly PyCharm TBH

lordmauve · 2018-01-03T22:27:28Z

adventurelib.py

+        pass
+
+
+class TerminalInterface(Interface):


I do like extracting this as a class. However adventurelib.say() should delegate to it.

Ah, that's a good idea, but I'm not certain how to do that. To be clear, the reason I have it set up this way is so that other interfaces- such as an interface to a webpage or a GUI- could be made. So just always delegating to TerminalInterface would break things.

lordmauve · 2018-01-03T22:30:02Z

test_adventurelib.py

        "\n"
        "And this is a second paragraph that is\n"
        "separately wrapped.\n"
    )


-def test_say_paragraph():


Well spotted here with the name duplication, but the tests are both intended to run. One tests two separate paragraphs of one source line each, one tests one paragraph of two source lines.

If a bug is introduced in say(), the distinction between which of these tests fails can help point to the reason.

lordmauve · 2018-01-03T22:38:55Z

demo_game.py

-def go(direction):
-    global current_room
-    room = current_room.exit(direction)
+def go(direction, game):


I don't like making every function take a 'game' parameter. It's not always needed. It would be better to make game a global object in the adventurelib namespace; then it's an entirely optional feature.

Indeed, this is really the philosophy of adventurelib - all features are optional. Items, Rooms, say(), help(), when() - they're all capabilities to make it easier to write a program, but you don't have to use any of them if you don't want to (and they don't force themselves on you).

If game is a global then you also don't need a setup kwarg to start() in order to be able to get hold of it prior to the game starting. You can just import it and set it up yourself, either in module scope or in a setup() function that you can re-use.

I believe I attempted to make it so that functions without a game parameter still work BTW, so it should still be possible to use the original design.

I believe that much of the reasoning for me behind not making game a global is that it allows the user to have multiple games running in the same file. While this may seem like an unnecessary feature, I can't tell you how many times I've been tripped up when I discovered that the library I'm using prevents me from having multiple of whatever-it-helps-me-to-implement in one program.

I agree that supporting multiple games from the same file would be better for more serious applications, but for education I just don't think it's necessary.

I could imagine making global state optional, by creating classes that represent all state that is currently global, and preserving the same API by having "default" instances of those classes. But really I think the extra complexity of that is not justified by its usefulness for education.

In particular, I did think about approaches for running adventurelib games in the browser, and while a backend webapp might be best to allow multiple games in the same interpreter, my preferred approach would be #11 - get the Python code working client-side in the browser.

lordmauve · 2018-01-03T22:41:36Z

adventurelib.py

@@ -30,6 +32,10 @@ class InvalidCommand(Exception):
    """A command is not defined correctly."""


+class InvalidDirection(Exception):


This is a good catch; I wonder how this got omitted.

lordmauve · 2018-01-03T22:53:57Z

adventurelib.py

+        print('\n\n'.join(formatted))
+
+
+class Game:


I broadly don't like this class. I don't like that it uses attribute access magic - and all the magic really achieves is keeping world variables separate from interface and say. But I don't think interface and say belong in this class.

What I'm saying is that a simpler class could achieve the same thing. Actually, even

class Game: pass

is workable for eliminating some of the global statements - and can be defined in user code.

That's a good enough plan I suppose.

ghost · 2018-01-06T01:05:14Z

What's your opinion of changing Room.<direction> to Room.exits.<direction>?

lordmauve · 2018-02-04T12:12:04Z

Sorry for slow responses, @hppavilion1, I've been planning a wedding and haven't had time to go through Github PRs.

KeyWeeUsr

Every file is chmodded in git repo to 0755 from the original 0644 which I don't think is a good idea at all. Doesn't matter whether it's Pycharm or educational purpose or whatever if the chmod doesn't have a real purpose (such as a shell script or shebang Python script or ...).

Brought code in line with PEP8, added (optional) Game World interface…

62944e0

…, updated demo to match.

lordmauve requested changes Jan 3, 2018

View reviewed changes

lordmauve added a commit that referenced this pull request Jan 3, 2018

Fix bugs identified by @hppavilion1 in #8

fa54c2f

KeyWeeUsr suggested changes Sep 15, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Brought code in line with PEP8, added (optional) Game World interface #8

Brought code in line with PEP8, added (optional) Game World interface #8

ghost commented Jan 3, 2018

lordmauve left a comment

lordmauve Jan 3, 2018

ghost Jan 6, 2018

lordmauve Jan 3, 2018

lordmauve Jan 3, 2018

ghost Jan 6, 2018

lordmauve Jan 3, 2018

ghost Jan 6, 2018

KeyWeeUsr Sep 15, 2018

lordmauve Jan 3, 2018

ghost Jan 6, 2018

lordmauve Jan 3, 2018

ghost Jan 6, 2018

lordmauve Jan 3, 2018

ghost Jan 6, 2018

lordmauve Jan 3, 2018

lordmauve Jan 3, 2018

ghost Jan 6, 2018 •

edited by ghost

Loading

ghost Jan 6, 2018

lordmauve Feb 4, 2018

lordmauve Jan 3, 2018

lordmauve Jan 3, 2018

ghost Jan 6, 2018

ghost commented Jan 6, 2018

lordmauve commented Feb 4, 2018

KeyWeeUsr left a comment

		@@ -30,6 +32,10 @@ class InvalidCommand(Exception):
		"""A command is not defined correctly."""


		class InvalidDirection(Exception):

Brought code in line with PEP8, added (optional) Game World interface #8

Are you sure you want to change the base?

Brought code in line with PEP8, added (optional) Game World interface #8

Conversation

ghost commented Jan 3, 2018

lordmauve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jan 6, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jan 6, 2018

lordmauve commented Feb 4, 2018

KeyWeeUsr left a comment

Choose a reason for hiding this comment

ghost Jan 6, 2018 •

edited by ghost

Loading