Merge lp:~jerith/divmod.org/short-directions-1215929 into lp:divmod.org

[Jeremy Thurgood]

Adds some short aliases for standard directions.

[lp:~jerith/divmod.org/short-directions-1215929]

2716. By Jeremy Thurgood on 2013-08-23

camelCase, not separated_with_slugs.

2717. By Jeremy Thurgood on 2013-08-23

Fix up some tests.

[Jean-Paul Calderone (exarkun)]

I wonder about the change to explicitly use `pyparsing.LineEnd()` in the expression for `Go`. Am I correct in concluding from this change that commands are currently allowed to match lines as long as the command is a *prefix* of the line (potentially leaving unmatched text at the end)? I can imagine this arising here since none of the existing commands are as like to be prefixes of other commands or bogus inputs as the new "n", "e", "s", and "w". If this is the case it might make sense to fix this more generally by declaring (to us, the development team) that `Action.expr` implicitly has `pyparsing.LineEnd()` at the end and make `Action` automatically put it there for us.

Otherwise I think this is generally okay, but a couple more hopefully-straightforward comments:

  - there are help files in Imaginary/imaginary/resources/ that should be kept up-to-date with respect to the actual implementation of commands.
  - the unit tests should be broken up more so that each one is only testing one case (I know this does not reflect the prevailing style in Imaginary's test suite, sorry :/)
  - I want to mumble something about twisted.python.constants but that can clearly be delayed for a later ticket

Thanks for the contribution!

[Jeremy Thurgood (jerith)]

Commands currently do match prefixes, and the general `pyparsing.LineEnd()` in actions is a reasonable way to avoid doing this accidentally. We'll still either need an explicit `pyparsing.LineEnd()` in the `Go` expression or we'll need to move the `DIRECTION_LITERAL` option to the end of the expression. I'm inclined to go with the latter.

[lp:~jerith/divmod.org/short-directions-1215929]

2718. By Jeremy Thurgood on 2013-09-22

More general end-of-line matching for actions.

2719. By Jeremy Thurgood on 2013-09-22

Update help text with direction aliases.

[Jean-Paul Calderone (exarkun)]

Thanks! This looks pretty good to me. A few points, all of which I'm happy to see addressed as follow-ups rather than directly in this branch:

  - There's still lots of room for shortening tests. A lot of dig, bury, and movement tests are still exercising multiple things in a single method.

  - There should be an explicit unit test for the new automatic `LineEnd` feature of Action.match

  - It occurs to me that the expression for `Go` is a bit repetitive. It might be nice to shorten that expression.

As I mentioned above, I don't think any of these are blockers for this branch. If you're happy with it as-is, please go ahead and merge (ping me if you don't have permission to do that yet). If you could file bugs for the above three points as well, that'd be great. :)

Thanks again.

[Jean-Paul Calderone (exarkun)]

review: Approve

=== modified file 'Imaginary/imaginary/action.py'
--- Imaginary/imaginary/action.py	2013-06-09 16:07:17 +0000
+++ Imaginary/imaginary/action.py	2013-09-22 10:10:54 +0000
@@ -160,15 +160,18 @@
     @classmethod
     def match(cls, player, line):
         """
-        @return: a list of 2-tuples of all the results of parsing the given
-            C{line} using this L{Action} type's pyparsing C{expr} attribute, or
-            None if the expression does not match the given line.
+        Parse the given C{line} using this L{Action} type's pyparsing C{expr}
+        attribute.  A C{pyparsing.LineEnd} is appended to C{expr} to avoid
+        accidentally matching a prefix instead of the whole line.
+
+        @return: a list of 2-tuples of all the results of parsing, or None if
+            the expression does not match the given line.
 
         @param line: a line of user input to be interpreted as an action.
 
         @see: L{imaginary.pyparsing}
         """
-        return cls.expr.parseString(line)
+        return (cls.expr + pyparsing.LineEnd()).parseString(line)
 
 
     def do(self, player, line, **slots):
@@ -788,11 +791,21 @@
 
 
 
+_directionNames = objects.OPPOSITE_DIRECTIONS.keys()
+_directionNames.extend(objects.DIRECTION_ALIASES.keys())
+
 DIRECTION_LITERAL = reduce(
     operator.xor, [
         pyparsing.Literal(d)
-        for d
-        in objects.OPPOSITE_DIRECTIONS]).setResultsName("direction")
+        for d in _directionNames]).setResultsName("direction")
+
+
+
+def expandDirection(direction):
+    """
+    Expand direction aliases into the names of the directions they refer to.
+    """
+    return objects.DIRECTION_ALIASES.get(direction, direction)
 
 
 
@@ -804,6 +817,7 @@
             pyparsing.restOfLine.setResultsName("name"))
 
     def do(self, player, line, direction, name):
+        direction = expandDirection(direction)
         if iimaginary.IContainer(player.thing.location).getExitNamed(direction, None) is not None:
             raise eimaginary.ActionFailure(events.ThatDoesntMakeSense(
                 actor=player.thing,
@@ -831,6 +845,7 @@
             DIRECTION_LITERAL)
 
     def do(self, player, line, direction):
+        direction = expandDirection(direction)
         for exit in iimaginary.IContainer(player.thing.location).getExits():
             if exit.name == direction:
                 if exit.sibling is not None:
@@ -858,13 +873,13 @@
 
 class Go(Action):
     expr = (
-        DIRECTION_LITERAL |
         (pyparsing.Literal("go") + pyparsing.White() +
          targetString("direction")) |
         (pyparsing.Literal("enter") + pyparsing.White() +
          targetString("direction")) |
         (pyparsing.Literal("exit") + pyparsing.White() +
-         targetString("direction")))
+         targetString("direction")) |
+        DIRECTION_LITERAL)
 
     actorInterface = iimaginary.IThing
 
@@ -873,6 +888,7 @@
         Identify a direction by having the player search for L{IExit}
         providers that they can see and reach.
         """
+        directionName = expandDirection(directionName)
         return player.obtainOrReportWhyNot(
             Proximity(
                 3.0,

=== modified file 'Imaginary/imaginary/objects.py'
--- Imaginary/imaginary/objects.py	2013-07-24 22:20:29 +0000
+++ Imaginary/imaginary/objects.py	2013-09-22 10:10:54 +0000
@@ -433,6 +433,18 @@
 
 
 
+DIRECTION_ALIASES = {
+    u"n": u"north",
+    u"s": u"south",
+    u"w": u"west",
+    u"e": u"east",
+    u"nw": u"northwest",
+    u"se": u"southeast",
+    u"ne": u"northeast",
+    u"sw": u"southwest"}
+
+
+
 class Exit(item.Item):
     """
     An L{Exit} is an oriented pathway between two L{Thing}s which each

=== modified file 'Imaginary/imaginary/resources/help/bury'
--- Imaginary/imaginary/resources/help/bury	2006-04-12 02:41:46 +0000
+++ Imaginary/imaginary/resources/help/bury	2013-09-22 10:10:54 +0000
@@ -2,4 +2,5 @@
 
 Usage: bury <north | south | east | west>
 
-Block off the exit in the indicated direction.
+Block off the exit in the indicated direction.  Short directions (n, s, e, w)
+may be used in place of the full direction names.

=== modified file 'Imaginary/imaginary/resources/help/dig'
--- Imaginary/imaginary/resources/help/dig	2006-04-12 02:41:46 +0000
+++ Imaginary/imaginary/resources/help/dig	2013-09-22 10:10:54 +0000
@@ -5,4 +5,5 @@
 Create a new location with the indicated name and create a two-way
 passage between it and the current location.  The exit will be in
 the specified direction from the current location to the new
-location.
+location.  Short directions (n, s, e, w) may be used in place of the
+full direction names.

=== modified file 'Imaginary/imaginary/resources/help/go'
--- Imaginary/imaginary/resources/help/go	2006-04-12 02:41:46 +0000
+++ Imaginary/imaginary/resources/help/go	2013-09-22 10:10:54 +0000
@@ -2,4 +2,5 @@
 
 Usage: go <north | south | east | west>
 
-Travel in the indicated direction.
+Travel in the indicated direction.  Short directions (n, s, e, w) may be used
+in place of the full direction names.

=== modified file 'Imaginary/imaginary/test/test_actions.py'
--- Imaginary/imaginary/test/test_actions.py	2013-07-26 00:20:17 +0000
+++ Imaginary/imaginary/test/test_actions.py	2013-09-22 10:10:54 +0000
@@ -423,7 +423,11 @@
         self.assertEquals(x[5].groups(), ())
 
 
-    def testDig(self):
+    def test_dig(self):
+        """
+        The I{dig} action creates a new location connected to the current
+        location through an exit in the specified direction.
+        """
         self._test(
             "dig west dark tunnel",
             ["You create an exit."],
@@ -447,45 +451,125 @@
             "dig west boring tunnel",
             ["There is already an exit in that direction."])
 
-    def testBury(self):
-        self._test(
-            "bury south",
-            ["There isn't an exit in that direction."])
-        self.assertEquals(list(iimaginary.IContainer(self.location).getExits()), [])
-
-        room = objects.Thing(store=self.store, name=u"destination", proper=True)
-        objects.Container.createFor(room, capacity=1000)
-        objects.Exit.link(room, self.location, u'north')
-
-        self._test(
-            "bury south",
-            ["It's gone."],
-            ["Test Player destroyed the exit to destination."])
-
-        self.assertEquals(
-            list(iimaginary.IContainer(self.location).getExits()),
-            [])
-
-        self.assertEquals(
-            list(iimaginary.IContainer(room).getExits()),
-            [])
-
-        objects.Exit.link(self.location, room, u'south')
-        self.observer.moveTo(room)
-
-        self._test(
-            "bury south",
-            ["It's gone."],
-            ["The exit to Test Location crumbles and disappears."])
-        self.assertEquals(
-            list(iimaginary.IContainer(self.location).getExits()),
-            [])
-        self.assertEquals(
-            list(iimaginary.IContainer(room).getExits()),
-            [])
-
-
-    def testGo(self):
+
+    def test_digWithDirectionAliases(self):
+        """
+        The I{dig} action creates a new location connected to the current
+        location through an exit in the specified direction even when that
+        direction is an alias.
+        """
+        self._test(
+            "dig w dark tunnel",
+            ["You create an exit."],
+            ["Test Player created an exit to the west."])
+        room = iimaginary.IContainer(self.location).getExitNamed(u'west').toLocation
+        self.assertEquals(room.name, u"dark tunnel")
+        self.assertEquals(room.description, u'')
+        self.assertIdentical(iimaginary.IContainer(room).getExitNamed(u'east').toLocation,
+                             self.location)
+
+        self._test(
+            "dig e bright tunnel",
+            ["You create an exit."],
+            ["Test Player created an exit to the east."])
+        room = iimaginary.IContainer(self.location).getExitNamed(u'east').toLocation
+        self.assertEquals(room.name, u"bright tunnel")
+        self.assertEquals(room.description, u'')
+        self.assertIdentical(iimaginary.IContainer(room).getExitNamed(u'west').toLocation, self.location)
+
+        self._test(
+            "dig w boring tunnel",
+            ["There is already an exit in that direction."])
+
+
+    def test_bury(self):
+        """
+        The I{bury} action destroys an exit in the specified direction.
+        """
+        self._test(
+            "bury south",
+            ["There isn't an exit in that direction."])
+        self.assertEquals(list(iimaginary.IContainer(self.location).getExits()), [])
+
+        room = objects.Thing(store=self.store, name=u"destination", proper=True)
+        objects.Container.createFor(room, capacity=1000)
+        objects.Exit.link(room, self.location, u'north')
+
+        self._test(
+            "bury south",
+            ["It's gone."],
+            ["Test Player destroyed the exit to destination."])
+
+        self.assertEquals(
+            list(iimaginary.IContainer(self.location).getExits()),
+            [])
+
+        self.assertEquals(
+            list(iimaginary.IContainer(room).getExits()),
+            [])
+
+        objects.Exit.link(self.location, room, u'south')
+        self.observer.moveTo(room)
+
+        self._test(
+            "bury south",
+            ["It's gone."],
+            ["The exit to Test Location crumbles and disappears."])
+        self.assertEquals(
+            list(iimaginary.IContainer(self.location).getExits()),
+            [])
+        self.assertEquals(
+            list(iimaginary.IContainer(room).getExits()),
+            [])
+
+
+    def test_buryWithDirectionAliases(self):
+        """
+        The I{bury} action destroys an exit in the specified direction even
+        when that direction is an alias.
+        """
+        self._test(
+            "bury s",
+            ["There isn't an exit in that direction."])
+        self.assertEquals(list(iimaginary.IContainer(self.location).getExits()), [])
+
+        room = objects.Thing(store=self.store, name=u"destination", proper=True)
+        objects.Container.createFor(room, capacity=1000)
+        objects.Exit.link(room, self.location, u'north')
+
+        self._test(
+            "bury s",
+            ["It's gone."],
+            ["Test Player destroyed the exit to destination."])
+
+        self.assertEquals(
+            list(iimaginary.IContainer(self.location).getExits()),
+            [])
+
+        self.assertEquals(
+            list(iimaginary.IContainer(room).getExits()),
+            [])
+
+        objects.Exit.link(self.location, room, u'south')
+        self.observer.moveTo(room)
+
+        self._test(
+            "bury s",
+            ["It's gone."],
+            ["The exit to Test Location crumbles and disappears."])
+        self.assertEquals(
+            list(iimaginary.IContainer(self.location).getExits()),
+            [])
+        self.assertEquals(
+            list(iimaginary.IContainer(room).getExits()),
+            [])
+
+
+    def test_go(self):
+        """
+        The I{go} action moves the player through an exit in the specified
+        direction.
+        """
         self._test(
             "go west",
             ["You can't go that way."])
@@ -516,6 +600,41 @@
             ["Test Player arrives from the west."])
 
 
+    def test_goThroughDirectionAliases(self):
+        """
+        The I{go} action moves the player through an exit in the specified
+        direction even when that direction is an alias.
+        """
+        self._test(
+            "go w",
+            ["You can't go that way."])
+        self._test(
+            "w",
+            ["You can't go that way."])
+
+        room = objects.Thing(store=self.store, name=u"destination")
+        objects.Container.createFor(room, capacity=1000)
+        objects.Exit.link(self.location, room, u"west")
+
+        self._test(
+            "w",
+            [E("[ destination ]"),
+             E("( east )"),
+             ""],
+            ["Test Player leaves west."])
+
+        self._test(
+            "n",
+            ["You can't go that way."])
+        self._test(
+            "go e",
+            [E("[ Test Location ]"),
+             E("( west )"),
+             "Location for testing.",
+             "Here, you see Observer Player."],
+            ["Test Player arrives from the west."])
+
+
     def test_goThroughOneWayExit(self):
         """
         Going through a one-way exit with a known direction will announce that
@@ -559,7 +678,11 @@
         self.assertCommandOutput("go east", [E("You can't go that way.")], [])
 
 
-    def testDirectionalMovement(self):
+    def test_directionalMovement(self):
+        """
+        You can move through exits in standard directions by just specifying
+        the direction.
+        """
         # A couple tweaks to state to make the test simpler
         self.observer.location = None
         self.location.description = None
@@ -579,6 +702,13 @@
                 [E("[ ") + ".*" + E(" ]"), # Not testing room description
                  E("( ") + ".*" + E(" )"), # Just do enough to make sure it was not an error.
                  ""])
+        shortDirections = ["nw", "n", "ne", "e", "w", "sw", "s", "se"]
+        for d, rd in zip(shortDirections, reversed(shortDirections)):
+            self._test(
+                d,
+                [E("[ ") + ".*" + E(" ]"), # Not testing room description
+                 E("( ") + ".*" + E(" )"), # Just do enough to make sure it was not an error.
+                 ""])
 
 
     def test_scrutinize(self):