Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .gitignore
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -91,3 +91,7 @@ ENV/
doc/_build

*.swp

# PyCharm IDE files
.git/
.idea/
128 changes: 102 additions & 26 deletions adventurelib.py
100644 → 100755
Original file line number Diff line number Diff line change
@@ -1,25 +1,27 @@
"""Simple library for writing Text Adventures in Python"""
import re
import sys
import inspect
import readline
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import textwrap
import random
from copy import deepcopy
from functools import partial
from itertools import zip_longest
try:
from shutil import get_terminal_size
except ImportError:
from backports.shutil_get_terminal_size import get_terminal_size
else:
def get_terminal_size(fallback=(80, 24)):
return fallback
try:
from backports.shutil_get_terminal_size import get_terminal_size
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

except ImportError:
def get_terminal_size(fallback=(80, 24)):
"""Fallback definition for terminal size getting"""
return fallback


__all__ = (
'when',
'start',
'Room',
'Pattern',
'_handle_command',
'Item',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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


class InvalidDirection(Exception):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""User attempts to travel in an invalid direction."""


class Placeholder:
"""Match a word in a command string."""
def __init__(self, name):
Expand All @@ -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):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if not direc.islower():
raise InvalidCommand(
'Invalid direction %r: directions must be all lowercase.'
)
if dir in Room._directions:
if direc in Room._directions:
raise KeyError('%r is already a direction!' % dir)
Room._directions[forward] = reverse
Room._directions[reverse] = forward
Expand Down Expand Up @@ -100,6 +106,7 @@ def __setattr__(self, name, value):
else:
object.__setattr__(self, name, value)


Room.add_direction('north', 'south')
Room.add_direction('east', 'west')

Expand Down Expand Up @@ -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)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was mostly PyCharm TBH


def take(self, name):
"""Remove an Item from the bag if it is present.
Expand Down Expand Up @@ -193,12 +200,14 @@ def take_random(self):
return obj


def _register(command, func, kwargs={}):
def _register(command, func, kwargs=None):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you're making this change because of a warning about mutable default arguments. But it's fine if there is no intention to change them. Here kwargs is not expected to ever be mutated, and the version I have is clearer in autogenerated documentation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that is why I did it really...

"""Register func as a handler for the given command."""
if kwargs is None:
kwargs = {}
pattern = Pattern(command)
sig = inspect.signature(func)
func_argnames = set(sig.parameters)
when_argnames = set(pattern.argnames) | set(kwargs.keys())
when_argnames = set(pattern.argnames) | set(kwargs.keys()) | {'game'}
if func_argnames != when_argnames:
raise InvalidCommand(
'The function %s%s has the wrong signature for @when(%r)' % (
Expand All @@ -212,6 +221,7 @@ def _register(command, func, kwargs={}):


class Pattern:
"""Command-matching pattern"""
def __init__(self, pattern):
self.orig_pattern = pattern
words = pattern.split()
Expand All @@ -221,7 +231,7 @@ def __init__(self, pattern):
for w in words:
if not w.isalpha():
raise InvalidCommand(
'Invalid command %r' % command +
'Invalid command %r' % w +
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original is what I intended. While pointing out the exact location of an issue adds value, removing the context of the rest of the command makes it harder to debug which command pattern has an error.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, misread this. This should have been pattern, not command or w.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. command wasn't defined anywhere, so I made my best guess.

'Commands may consist of letters only.'
)
if w.isupper():
Expand All @@ -233,7 +243,7 @@ def __init__(self, pattern):
match.append(w)
else:
raise InvalidCommand(
'Invalid command %r' % command +
'Invalid command %r' % w +
'\n\nWords in commands must either be in lowercase or ' +
'capitals, not a mix.'
)
Expand All @@ -254,6 +264,7 @@ def __repr__(self):

@staticmethod
def word_combinations(have, placeholders):
"""??? (not sure what this does)"""
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then please don't add a docstring.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was honestly just trying to say "can you add a docstring to explain", though the subtlety was likely lost in transcription.

if have < placeholders:
return
if have == placeholders:
Expand All @@ -272,9 +283,10 @@ def word_combinations(have, placeholders):
combos = Pattern.word_combinations(remain, other_groups)
for buckets in combos:
yield (take,) + tuple(buckets)
take -= 1 # backtrack
take -= 1 # backtrack

def match(self, input_words):
"""Attempt to match a command against the pattern"""
if len(input_words) < len(self.argnames):
return None

Expand Down Expand Up @@ -326,55 +338,119 @@ def no_command_matches(command):
def when(command, **kwargs):
"""Decorator for command functions."""
def dec(func):
"""decorator"""
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This docstring doesn't add any information.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...yeah, I just have my IDE set to complain to me when I don't have a docstring to encourage me to document my code better, and I can get a little overzealous. Sorry.

_register(command, func, kwargs)
return func
return dec


def help():
def cmd_help():
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function name is part of the public API and must not be changed. Adventurelib uses short, simple names like help() and say() to be more accessible to younger programmers.

help() is only really useful in the __main__ namespace in the REPL; it's not useful to care if it's shadowed in a library.

"""Print a list of the commands you can give."""
print('Here is a list of the commands you can give:')
cmds = sorted(c.orig_pattern for c, _, _ in commands)
for c in cmds:
print(c)


def _handle_command(cmd):
def _handle_command(cmd, game=None):
"""Handle a command typed by the user."""
ws = cmd.lower().split()
for pattern, func, kwargs in commands:
args = kwargs.copy()
matches = pattern.match(ws)
if matches is not None:
args.update(matches)
func(**args)
func(**args, game=game)
break
else:
no_command_matches(cmd)
print()


def start(help=True):
class Interface:
"""Superclass for all interfaces (ways of interacting with the game)"""
def __init__(self):
pass

def get_command(self, prompt):
"""Get a command"""
return ''

def say(self, msg):
"""Send output to the user"""
pass


class TerminalInterface(Interface):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""Interface for basic terminal I/O (the default)"""
def get_command(self, prompt):
"""Get a command"""
return input(prompt).strip()

def say(self, msg):
"""Print a message.

Unlike print(), this deals with de-denting and wrapping of text to fit
within the width of the terminal.

Paragraphs separated by blank lines in the input will be wrapped
separately.

"""
msg = str(msg)
msg = re.sub(r'^[ \t]*(.*?)[ \t]*$', r'\1', msg, flags=re.M)
width = get_terminal_size()[0]
paragraphs = re.split(r'\n(?:[ \t]*\n)', msg)
formatted = (textwrap.fill(p.strip(), width=width) for p in paragraphs)
print('\n\n'.join(formatted))


class Game:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good enough plan I suppose.

"""Game World Environment"""
def __init__(self, interface: Interface):
self.worldvars = {}
self.interface = interface
self.say = self.interface.say

def __getattr__(self, item):
try:
return self.worldvars[item]
except KeyError:
raise AttributeError()

def __setattr__(self, attr, val):
if attr in {'worldvars', 'interface', 'say'}:
print(' in dict')
self.__dict__[attr] = val
else:
self.worldvars[attr] = val


def start(setup=None, interface: Interface=TerminalInterface(), help=True):
"""Run the game."""
if help:
# Ugly, but we want to keep the arguments consistent
help = globals()['help']
qmark = Pattern('help')
qmark.prefix = ['?']
qmark.orig_pattern = '?'
commands.insert(0, (Pattern('help'), help, {}))
commands.insert(0, (qmark, help, {}))
commands.insert(0, (Pattern('help'), cmd_help, {}))
commands.insert(0, (qmark, cmd_help, {}))

game = Game(interface)

if setup is not None:
setup(game)

while True:
try:
cmd = input(prompt()).strip()
cmd = interface.get_command(prompt())
except EOFError:
print()
break

if not cmd:
continue

_handle_command(cmd)
_handle_command(cmd, game)


def say(msg):
Expand Down
66 changes: 35 additions & 31 deletions demo_game.py
100644 → 100755
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
"""Demonstration game"""
from adventurelib import *

Room.items = Bag()

current_room = starting_room = Room("""
starting_room = Room("""
You are in a dark room.
""")

Expand All @@ -11,58 +12,61 @@
""")

mallet = Item('rusty mallet', 'mallet')
valley.items = Bag({mallet,})

inventory = Bag()
valley.items = Bag({mallet, })


@when('north', direction='north')
@when('south', direction='south')
@when('east', direction='east')
@when('west', direction='west')
def go(direction):
global current_room
room = current_room.exit(direction)
def go(direction, game):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@ghost ghost Jan 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

room = game.current_room.exit(direction)
if room:
current_room = room
say('You go %s.' % direction)
look()
game.current_room = room
game.say('You go %s.' % direction)
look(game)


@when('take ITEM')
def take(item):
obj = current_room.items.take(item)
def take(item, game):
obj = game.current_room.items.take(item)
if obj:
say('You pick up the %s.' % obj)
inventory.add(obj)
game.say('You pick up the %s.' % obj)
game.inventory.add(obj)
else:
say('There is no %s here.' % item)
game.say('There is no %s here.' % item)


@when('drop THING')
def drop(thing):
obj = inventory.take(thing)
def drop(thing, game):
obj = game.inventory.take(thing)
if not obj:
say('You do not have a %s.' % thing)
game.say('You do not have a %s.' % thing)
else:
say('You drop the %s.' % obj)
current_room.items.add(obj)
game.say('You drop the %s.' % obj)
game.current_room.items.add(obj)


@when('look')
def look():
say(current_room)
if current_room.items:
for i in current_room.items:
say('A %s is here.' % i)
def look(game):
game.say(game.current_room)
if game.current_room.items:
for i in game.current_room.items:
game.say('A %s is here.' % i)


@when('inventory')
def show_inventory():
say('You have:')
for thing in inventory:
say(thing)
def show_inventory(game):
game.say('You have:')
for thing in game.inventory:
game.say(thing)


def setup(game):
"""Set up the game world"""
game.current_room = starting_room
game.inventory = Bag()
look(game)


look()
start()
start(setup)
Loading