Merge lp:~abentley/bzr-builder/usage into lp:bzr-builder

[Aaron Bentley]

Provide USAGE in parse errors.

This branch adds usage info to bzr-builder's instruction parsing errors
to make it easier to fix incorrect syntax.

[Martin Pool (mbp)]

That looks reasonable to me, though James might like to review it too.

Needing to pass around the 'instruction' quite so much suggests perhaps splitting out a class that knows the instruction it's parsing?

review: Approve

[James Westby (james-w)]

review: Approve

=== modified file 'recipe.py'
--- recipe.py	2010-10-27 18:22:03 +0000
+++ recipe.py	2010-11-18 21:01:47 +0000
@@ -16,6 +16,7 @@
 import os
 import signal
 import subprocess
+from textwrap import dedent
 
 from bzrlib import (
     branch,
@@ -55,6 +56,10 @@
 NEST_PART_INSTRUCTION = "nest-part"
 NEST_INSTRUCTION = "nest"
 RUN_INSTRUCTION = "run"
+USAGE = {
+    'merge': 'merge NAME BRANCH [REVISION]',
+    'nest': 'nest NAME BRANCH TARGET-DIR [REVISION]',
+    'nest-part': 'NAME BRANCH SUBDIR [TARGET-DIR [REVISION]]',}
 
 SAFE_INSTRUCTIONS = [
     MERGE_INSTRUCTION, NEST_PART_INSTRUCTION, NEST_INSTRUCTION]
@@ -767,6 +772,14 @@
                 problem=problem)
 
 
+class InstructionParseError(RecipeParseError):
+    _fmt = RecipeParseError._fmt + "\nUsage: %(usage)s"
+
+    def __init__(self, filename, line, char, problem, instruction):
+        RecipeParseError.__init__(self, filename, line, char, problem)
+        self.usage = USAGE[instruction]
+
+
 class ForbiddenInstructionError(RecipeParseError):
 
     def __init__(self, filename, line, char, problem, instruction_name=None):
@@ -858,12 +871,12 @@
                     self.new_line()
                     active_branches[-1].run_command(command)
                 else:
-                    branch_id = self.parse_branch_id()
-                    url = self.parse_branch_url()
+                    branch_id = self.parse_branch_id(instruction)
+                    url = self.parse_branch_url(instruction)
                     if instruction == NEST_INSTRUCTION:
-                        location = self.parse_branch_location()
+                        location = self.parse_branch_location(instruction)
                     if instruction == NEST_PART_INSTRUCTION:
-                        path = self.parse_subpath()
+                        path = self.parse_subpath(instruction)
                         target_subdir = self.parse_optional_path()
                         if target_subdir == '':
                             target_subdir = None
@@ -872,7 +885,7 @@
                             revspec = self.parse_optional_revspec()
                     else:
                         revspec = self.parse_optional_revspec()
-                    self.new_line()
+                    self.new_line(instruction)
                     last_branch = RecipeBranch(branch_id, url, revspec=revspec)
                     if instruction == NEST_INSTRUCTION:
                         active_branches[-1].nest_branch(location, last_branch)
@@ -927,12 +940,13 @@
         self.throw_parse_error("Expecting %s, got '%s'"
                 % (options_str, instruction))
 
-    def parse_branch_id(self):
-        self.parse_whitespace("the branch id")
+    def parse_branch_id(self, instruction):
+        self.parse_whitespace("the branch id", instruction=instruction)
         branch_id = self.peek_to_whitespace()
         if branch_id is None:
             self.throw_parse_error("End of line while looking for the "
-                    "branch id")
+                    "branch id", cls=InstructionParseError,
+                    instruction=instruction)
         if branch_id in self.seen_nicks:
             self.throw_parse_error("'%s' was already used to identify "
                     "a branch." % branch_id)
@@ -940,36 +954,39 @@
         self.seen_nicks.add(branch_id)
         return branch_id
 
-    def parse_branch_url(self):
-        self.parse_whitespace("the branch url")
-        branch_url = self.take_to_whitespace("the branch url")
+    def parse_branch_url(self, instruction):
+        self.parse_whitespace("the branch url", instruction=instruction)
+        branch_url = self.take_to_whitespace("the branch url", instruction)
         return branch_url
 
-    def parse_branch_location(self):
+    def parse_branch_location(self, instruction):
         # FIXME: Needs a better term
         self.parse_whitespace("the location to nest")
         location = self.peek_to_whitespace()
         if location is None:
             self.throw_parse_error("End of line while looking for the "
-                    "location to nest")
+                    "location to nest", cls=InstructionParseError,
+                    instruction=instruction)
         norm_location = os.path.normpath(location)
         if norm_location in self.seen_paths:
             self.throw_parse_error("The path '%s' is a duplicate of "
                     "the one used on line %d." % (location,
-                        self.seen_paths[norm_location]))
+                        self.seen_paths[norm_location]),
+                    InstructionParseError, instruction=instruction)
         if os.path.isabs(norm_location):
             self.throw_parse_error("Absolute paths are not allowed: %s"
-                    % location)
+                    % location, InstructionParseError, instruction=instruction)
         if norm_location.startswith(".."):
             self.throw_parse_error("Paths outside the current directory "
-                    "are not allowed: %s" % location)
+                    "are not allowed: %s" % location,
+                    cls=InstructionParseError, instruction=instruction)
         self.take_chars(len(location))
         self.seen_paths[norm_location] = self.line_index + 1
         return location
 
-    def parse_subpath(self):
-        self.parse_whitespace("the subpath to merge")
-        location = self.take_to_whitespace("the subpath to merge")
+    def parse_subpath(self, instruction):
+        self.parse_whitespace("the subpath to merge", instruction=instruction)
+        location = self.take_to_whitespace("the subpath to merge", instruction)
         return location
 
     def parse_revspec(self):
@@ -1004,13 +1021,18 @@
     def throw_eol(self, expected):
         self.throw_parse_error("End of line while looking for '%s'" % expected)
 
-    def new_line(self):
+    def new_line(self, instruction=None):
         # Jump over any whitespace
         self.parse_whitespace(None, require=False)
         remaining = self.peek_to_whitespace()
         if remaining != None:
+            kwargs = {}
+            if instruction is not None:
+                kwargs = {
+                    'cls': InstructionParseError,
+                    'instruction': instruction}
             self.throw_parse_error("Expecting the end of the line, got '%s'"
-                    % remaining)
+                    % remaining, **kwargs)
         self.index = 0
         self.line_index += 1
         if self.line_index >= len(self.lines):
@@ -1076,12 +1098,18 @@
            return old_indent_level
         return None
 
-    def parse_whitespace(self, looking_for, require=True):
+    def parse_whitespace(self, looking_for, require=True, instruction=None):
         if require:
             actual = self.peek_char()
             if actual is None:
+                kwargs = {}
+                if instruction is not None:
+                    kwargs = {
+                        'cls': InstructionParseError,
+                        'instruction': instruction,
+                        }
                 self.throw_parse_error("End of line while looking for "
-                        "%s" % looking_for)
+                        "%s" % looking_for, **kwargs)
             if actual not in self.whitespace_chars:
                 self.throw_parse_error("Expecting whitespace before %s, "
                         "got '%s'." % (looking_for, actual))
@@ -1128,11 +1156,16 @@
             char = self.peek_char(skip=count)
         return ret
 
-    def take_to_whitespace(self, looking_for):
+    def take_to_whitespace(self, looking_for, instruction=None):
         text = self.peek_to_whitespace()
         if text is None:
+            kwargs = {}
+            if instruction is not None:
+                kwargs = {
+                    'cls': InstructionParseError,
+                    'instruction': instruction}
             self.throw_parse_error("End of line while looking for %s"
-                    % looking_for)
+                    % looking_for, **kwargs)
         self.take_chars(len(text))
         return text
 

=== modified file 'tests/test_recipe.py'
--- tests/test_recipe.py	2010-10-27 18:22:03 +0000
+++ tests/test_recipe.py	2010-11-18 21:01:47 +0000
@@ -33,9 +33,11 @@
         pull_or_branch,
         RecipeParser,
         RecipeBranch,
+        InstructionParseError,
         RecipeParseError,
         resolve_revisions,
         SAFE_INSTRUCTIONS,
+        USAGE,
         )
 
 
@@ -59,6 +61,13 @@
         self.assertEqual("recipe", exc.filename)
         return exc
 
+    def assertInstructionParseError(self, line, char, problem, usage,
+            callable, *args, **kwargs):
+        exc = self.assertParseError(line, char, problem, callable, *args,
+                **kwargs)
+        self.assertIsInstance(exc, InstructionParseError),
+        self.assertEqual(usage, exc.usage)
+
     def check_recipe_branch(self, branch, name, url, revspec=None,
             num_child_branches=0, revid=None):
         self.assertEqual(name, branch.name)
@@ -138,53 +147,66 @@
                 self.basic_header + "http://foo.org/\n" + "cat")
 
     def test_rejects_merge_no_name(self):
-        self.assertParseError(3, 7, "End of line while looking for "
-                "the branch id", self.get_recipe,
-                self.basic_header_and_branch + "merge ")
+        self.assertInstructionParseError(3, 7, "End of line while looking for "
+                "the branch id", "merge NAME BRANCH [REVISION]",
+                self.get_recipe, self.basic_header_and_branch + "merge ")
+
+    def test_rejects_merge_no_name_no_space(self):
+        self.assertInstructionParseError(3, 6, "End of line while looking for "
+                "the branch id", "merge NAME BRANCH [REVISION]",
+                self.get_recipe, self.basic_header_and_branch + "merge")
 
     def test_rejects_merge_no_url(self):
-        self.assertParseError(3, 11, "End of line while looking for "
-                "the branch url", self.get_recipe,
-                self.basic_header_and_branch + "merge foo ")
+        self.assertInstructionParseError(3, 11, "End of line while looking for"
+                " the branch url", "merge NAME BRANCH [REVISION]",
+                self.get_recipe, self.basic_header_and_branch + "merge foo ")
 
     def test_rejects_text_at_end_of_merge_line(self):
-        self.assertParseError(3, 17, "Expecting the end of the line, "
-                "got 'bar'", self.get_recipe,
+        self.assertInstructionParseError(3, 17,
+                "Expecting the end of the line, got 'bar'",
+                'merge NAME BRANCH [REVISION]', self.get_recipe,
                 self.basic_header_and_branch + "merge foo url 2 bar")
 
     def test_rejects_nest_part_no_name(self):
-        self.assertParseError(3, 11, "End of line while looking for "
-                "the branch id", self.get_recipe,
+        self.assertInstructionParseError(3, 11, "End of line while looking for"
+                " the branch id", USAGE['nest-part'], self.get_recipe,
                 self.basic_header_and_branch + "nest-part ")
 
     def test_rejects_nest_part_no_url(self):
-        self.assertParseError(3, 15, "End of line while looking for "
-                "the branch url", self.get_recipe,
+        self.assertInstructionParseError(3, 15, "End of line while looking for"
+                " the branch url", USAGE['nest-part'], self.get_recipe,
                 self.basic_header_and_branch + "nest-part foo ")
 
     def test_rejects_nest_part_no_subpath(self):
-        self.assertParseError(3, 22, "End of line while looking for "
-                "the subpath to merge", self.get_recipe,
+        self.assertInstructionParseError(3, 22, "End of line while looking for"
+                " the subpath to merge", USAGE['nest-part'], self.get_recipe,
                 self.basic_header_and_branch + "nest-part foo url:// ")
 
+    def test_rejects_nest_part_no_subpath_no_space(self):
+        self.assertInstructionParseError(3, 21, "End of line while looking for"
+                " the subpath to merge", USAGE['nest-part'], self.get_recipe,
+                self.basic_header_and_branch + "nest-part foo url://")
+
     def test_rejects_nest_no_name(self):
-        self.assertParseError(3, 6, "End of line while looking for "
-                "the branch id", self.get_recipe,
+        self.assertInstructionParseError(3, 6, "End of line while looking for"
+                " the branch id", USAGE['nest'], self.get_recipe,
                 self.basic_header_and_branch + "nest ")
 
     def test_rejects_nest_no_url(self):
-        self.assertParseError(3, 10, "End of line while looking for "
-                "the branch url", self.get_recipe,
-                self.basic_header_and_branch + "nest foo ")
+        self.assertInstructionParseError(3, 10,
+            "End of line while looking for the branch url", USAGE['nest'],
+            self.get_recipe, self.basic_header_and_branch + "nest foo ")
 
     def test_rejects_nest_no_location(self):
-        self.assertParseError(3, 14, "End of line while looking for "
-                "the location to nest", self.get_recipe,
-                self.basic_header_and_branch + "nest foo url ")
+        self.assertInstructionParseError(3, 14,
+                "End of line while looking for the location to nest",
+                USAGE['nest'], self.get_recipe, self.basic_header_and_branch +
+                "nest foo url ")
 
     def test_rejects_text_at_end_of_nest_line(self):
-        self.assertParseError(3, 20, "Expecting the end of the line, "
-                "got 'baz'", self.get_recipe,
+        self.assertInstructionParseError(3, 20,
+                "Expecting the end of the line, got 'baz'", USAGE['nest'],
+                self.get_recipe,
                 self.basic_header_and_branch + "nest foo url bar 2 baz")
 
     def test_rejects_indent_after_first_branch(self):
@@ -443,34 +465,40 @@
         self.assertEqual("run", exc.instruction_name)
 
     def test_error_on_duplicate_path(self):
-        exc = self.assertParseError(3, 15, "The path '.' is a duplicate "
-                "of the one used on line 1.", self.get_recipe,
+        exc = self.assertInstructionParseError(3, 15,
+                "The path '.' is a duplicate of the one used on line 1.",
+                USAGE['nest'], self.get_recipe,
                 self.basic_header_and_branch + "nest nest url .\n")
 
     def test_error_on_duplicate_path_with_another_nest(self):
-        exc = self.assertParseError(4, 16, "The path 'foo' is a duplicate "
-                "of the one used on line 3.", self.get_recipe,
+        exc = self.assertInstructionParseError(4, 16,
+                "The path 'foo' is a duplicate of the one used on line 3.",
+                USAGE['nest'], self.get_recipe,
                 self.basic_header_and_branch + "nest nest url foo\n"
                 + "nest nest2 url foo\n")
 
     def test_duplicate_path_check_uses_normpath(self):
-        exc = self.assertParseError(3, 15, "The path 'foo/..' is a duplicate "
-                "of the one used on line 1.", self.get_recipe,
+        exc = self.assertInstructionParseError(3, 15,
+                "The path 'foo/..' is a duplicate of the one used on line 1.",
+                USAGE['nest'], self.get_recipe,
                 self.basic_header_and_branch + "nest nest url foo/..\n")
 
     def test_error_absolute_path(self):
-        exc = self.assertParseError(3, 15, "Absolute paths are not allowed: "
-                "/etc/passwd", self.get_recipe, self.basic_header_and_branch
+        exc = self.assertInstructionParseError(3, 15,
+                "Absolute paths are not allowed: /etc/passwd", USAGE['nest'],
+                self.get_recipe, self.basic_header_and_branch
                 + "nest nest url /etc/passwd\n")
 
     def test_error_simple_parent_dir(self):
-        exc = self.assertParseError(3, 15, "Paths outside the current "
-                "directory are not allowed: ../foo", self.get_recipe,
+        exc = self.assertInstructionParseError(3, 15,
+                "Paths outside the current directory are not allowed: ../foo",
+                USAGE['nest'], self.get_recipe,
                 self.basic_header_and_branch + "nest nest url ../foo\n")
 
     def test_error_complex_parent_dir(self):
-        exc = self.assertParseError(3, 15, "Paths outside the current "
-                "directory are not allowed: ./foo/../..", self.get_recipe,
+        exc = self.assertInstructionParseError(3, 15,
+                "Paths outside the current directory are not allowed:"
+                " ./foo/../..", USAGE['nest'], self.get_recipe,
                 self.basic_header_and_branch + "nest nest url ./foo/../..\n")
 
 
@@ -1064,8 +1092,8 @@
         nested_branch1 = RecipeBranch("nested1", "nested1_url",
                 revspec="tag:foo")
         base_branch.nest_branch(".", nested_branch1)
-        self.assertRaises(RecipeParseError, base_branch.get_recipe_text,
-                validate=True)
+        self.assertRaises(InstructionParseError,
+            base_branch.get_recipe_text, validate=True)
 
     def test_str_validates(self):
         base_branch = BaseRecipeBranch("base_url", "1", 0.1)
@@ -1073,7 +1101,7 @@
         nested_branch1 = RecipeBranch("nested1", "nested1_url",
                 revspec="tag:foo")
         base_branch.nest_branch(".", nested_branch1)
-        self.assertRaises(RecipeParseError, str, base_branch)
+        self.assertRaises(InstructionParseError, str, base_branch)
 
     def test_with_nest_part(self):
         base_branch = BaseRecipeBranch("base_url", "1", 0.1)