Introduce specific exceptions for buck project builder

Summary: Isolating the number of exceptions to catch should simplify the interface when using the builder. Also makes exceptions more debuggable.

Reviewed By: dkgi

Differential Revision: D14759548

fbshipit-source-id: 34cd00412c2f18b4d60c340ca14dbfc8e29284b4

Author: Matt D'Souza

client/buck_project_builder/__init__.py

@@ -17,6 +17,10 @@
 Target = NamedTuple("Target", [("build_file_directory", str), ("name", str)])
 
 
+class BuilderException(Exception):
+    pass
+
+
 class Builder(object):
     def __init__(self, buck_root: str, build_file_name: str = "TARGETS") -> None:
         self.buck_root = buck_root
@@ -53,14 +57,16 @@ def compute_targets_to_build(self, target: str) -> List[BuildTarget]:
             targets_seen.update(new_targets)
 
         if targets_not_found:
-            raise ValueError(
+            raise BuilderException(
                 "Target(s) not found: {}".format(", ".join(targets_not_found))
             )
         return build_targets
 
     def _parse_target(self, target: str) -> Target:
         if target.endswith("...") or target.endswith(":"):
-            raise ValueError("Target {} is not an absolute target.".format(target))
+            raise BuilderException(
+                "Target {} is not an absolute target.".format(target)
+            )
         split = _strip(target, left="//").split(":")
         return Target(split[0], split[1])
 

client/buck_project_builder/parser/__init__.py

@@ -17,6 +17,10 @@
 )
 
 
+class ParserException(Exception):
+    pass
+
+
 class Parser(object):
     def __init__(self, buck_root: str, build_file_name: str = "TARGETS") -> None:
         self.buck_root = buck_root
@@ -27,17 +31,15 @@ def parse_file(self, build_file_directory: str) -> BuildFile:
         if build_file_directory in self.build_files:
             return self.build_files[build_file_directory]
 
-        build_file_path = os.path.join(
-            self.buck_root, build_file_directory, self.build_file_name
-        )
+        build_file_path = self._build_file_path(build_file_directory)
         try:
             with open(build_file_path, "r") as build_file:
                 file_contents = build_file.read()
                 tree = ast.parse(file_contents)
         except FileNotFoundError:
-            raise ValueError("No build file found at {}.".format(build_file_path))
+            raise ParserException("No build file found at {}.".format(build_file_path))
         except SyntaxError as error:
-            raise ValueError(
+            raise ParserException(
                 "Could not parse build file at {}. Reason: {}".format(
                     build_file_path, error
                 )
@@ -48,6 +50,9 @@ def parse_file(self, build_file_directory: str) -> BuildFile:
         self.build_files[build_file_directory] = build_file
         return build_file
 
+    def _build_file_path(self, build_file_directory: str) -> str:
+        return os.path.join(self.buck_root, build_file_directory, self.build_file_name)
+
     def _parse_targets(
         self, tree: ast.AST, build_file_directory: str
     ) -> Dict[str, BuildTarget]:
@@ -56,14 +61,25 @@ def _parse_targets(
 
         targets = {}
         for expression in expressions:
-            assert isinstance(expression, ast.Expr)
-            call = expression.value
-            assert isinstance(call, ast.Call)
-            named = call.func
-            assert isinstance(named, ast.Name)
-            rule = named.id
-            if rule in SUPPORTED_RULES:
-                target = SUPPORTED_RULES[rule].parse(call, build_file_directory)
-                targets[target.name] = target
+            try:
+                assert isinstance(expression, ast.Expr)
+                call = expression.value
+                assert isinstance(call, ast.Call)
+                named = call.func
+                assert isinstance(named, ast.Name)
+                rule = named.id
+                if rule in SUPPORTED_RULES:
+                    target = SUPPORTED_RULES[rule].parse(call, build_file_directory)
+                    targets[target.name] = target
+            except (AssertionError, ValueError) as error:
+                raise ParserException(
+                    "Error occurred parsing targets in file {}, "
+                    "at line {}, column {}: {}".format(
+                        self._build_file_path(build_file_directory),
+                        expression.lineno,
+                        expression.col_offset,
+                        error,
+                    )
+                )
 
         return targets

client/buck_project_builder/parser/tests/parser_test.py

@@ -6,7 +6,7 @@
 import unittest
 from unittest.mock import mock_open, patch
 
-from .. import Parser
+from .. import Parser, ParserException
 from ..build_target import PythonBinary, PythonLibrary, PythonUnitTest
 
 
@@ -47,6 +47,12 @@
 )
 """
 
+TARGETS_FILE_2 = """
+python_binary(
+    name = 1234,
+)
+"""
+
 
 class ParserTest(unittest.TestCase):
     def test_parse_file(self):
@@ -92,3 +98,9 @@ def test_parse_file(self):
             # The parser should cache files it has already parsed.
             parser.parse_file("my/module")
             mocked_open.assert_not_called()
+
+        with patch("builtins.open", mock_open(read_data=TARGETS_FILE_2)) as mocked_open:
+            self.assertRaises(ParserException, parser.parse_file, "my/other_module")
+            mocked_open.assert_called_once_with(
+                "/buck_root/my/other_module/TARGETS", "r"
+            )

client/buck_project_builder/tests/builder_test.py

@@ -8,7 +8,7 @@
 from typing import List
 from unittest.mock import MagicMock, patch
 
-from .. import Builder, parser
+from .. import Builder, BuilderException, parser
 from ..parser.build_target import BuildTarget, PythonBinary, PythonLibrary
 
 
@@ -62,7 +62,7 @@ def test_compute_targets_to_build_simple(self):
             self.assert_targets_equal(targets, ["//project:e"])
 
             self.assertRaises(
-                ValueError, builder.compute_targets_to_build, "//project:f"
+                BuilderException, builder.compute_targets_to_build, "//project:f"
             )
 
     def test_compute_targets_to_build_complex(self):
@@ -116,7 +116,7 @@ def test_compute_targets_to_build_complex(self):
             self.assert_targets_equal(targets, ["//project2:e", "//project2:d"])
 
             self.assertRaises(
-                ValueError, builder.compute_targets_to_build, "//project1:f"
+                BuilderException, builder.compute_targets_to_build, "//project1:f"
             )
 
     def test_targets_to_build_file_wildcard(self):