Merge branch 'master' of github.com:pre-commit/pre-commit

Conflicts:
	tests/git_test.py

example_manifest.yaml

@@ -1,31 +1,29 @@
 
 # Hooks are set up as follows
-# hooks:
-#     -
-#         id: hook_id
-#         name: 'Readable name'
-#         entry: my_hook_executable
+# -
+#     id: hook_id
+#     name: 'Readable name'
+#     entry: my_hook_executable
 #
-#         # Optional
-#         description: 'Longer description of the hook'
+#     # Optional
+#     description: 'Longer description of the hook'
 #
-#         # Optional, for now 'python[optional version]', 'ruby #.#.#', 'node'
-#         language: 'python'
+#     # Optional, for now 'python[optional version]', 'ruby #.#.#', 'node'
+#     language: 'python'
 #
-#         # Optional, defaults to zero
-#         expected_return_value: 0
+#     # Optional, defaults to zero
+#     expected_return_value: 0
 
-hooks:
-    -
-        id: my_hook
-        name: My Simple Hook
-        description: This is my simple hook that does blah
-        entry: my-simple-hook.py
-        language: python
-        expected_return_value: 0
-    -
-        id: my_grep_based_hook
-        name: My Bash Based Hook
-        description: This is a hook that uses grep to validate some stuff
-        entry: ./my_grep_based_hook.sh
-        expected_return_value: 1
+-
+    id: my_hook
+    name: My Simple Hook
+    description: This is my simple hook that does blah
+    entry: my-simple-hook.py
+    language: python
+    expected_return_value: 0
+-
+    id: my_grep_based_hook
+    name: My Bash Based Hook
+    description: This is a hook that uses grep to validate some stuff
+    entry: ./my_grep_based_hook.sh
+    expected_return_value: 1

manifest.yaml

@@ -0,0 +1,7 @@
+
+-
+    id: validate_manifest
+    name: Validate Manifest
+    description: This validator validates a pre-commit hooks manifest file
+    entry: validate-manifest
+    language: python

pre_commit/clientlib/validate_base.py

@@ -0,0 +1,51 @@
+
+import jsonschema
+import jsonschema.exceptions
+import os.path
+import yaml
+
+from pre_commit import git
+
+
+def get_validator(
+    default_filename,
+    json_schema,
+    exception_type,
+    additional_validation_strategy=lambda obj: None,
+):
+    """Returns a function which will validate a yaml file for correctness
+
+    Args:
+        default_filename - Default filename to look for if none is specified
+        json_schema - JSON schema to validate file with
+        exception_type - Error type to raise on failure
+        additional_validation_strategy - Strategy for additional validation of
+            the object read from the file.  The function should either raise
+            exception_type on failure.
+    """
+
+    def validate(filename=None):
+        filename = filename or os.path.join(git.get_root(), default_filename)
+
+        if not os.path.exists(filename):
+            raise exception_type('File {0} does not exist'.format(filename))
+
+        file_contents = open(filename, 'r').read()
+
+        try:
+            obj = yaml.load(file_contents)
+        except Exception as e:
+            raise exception_type(
+                'File {0} is not a valid yaml file'.format(filename), e,
+            )
+
+        try:
+            jsonschema.validate(obj, json_schema)
+        except jsonschema.exceptions.ValidationError as e:
+            raise exception_type(
+                'File {0} is not a valid file'.format(filename), e,
+            )
+
+        additional_validation_strategy(obj)
+
+    return validate

pre_commit/clientlib/validate_manifest.py

@@ -2,47 +2,34 @@
 from __future__ import print_function
 
 import argparse
-import jsonschema
-import jsonschema.exceptions
-import os.path
-import yaml
 
 import pre_commit.constants as C
+from pre_commit.clientlib.validate_base import get_validator
 
 
 class InvalidManifestError(ValueError): pass
 
 
 MANIFEST_JSON_SCHEMA = {
-    'type': 'object',
-    'properties': {
-        'hooks': {
-            'type': 'array',
-            'minItems': 1,
-            'items': {
-                'type': 'object',
-                'properties': {
-                    'id': {'type': 'string'},
-                    'name': {'type': 'string'},
-                    'description': {'type': 'string'},
-                    'entry': {'type': 'string'},
-                    'language': {'type': 'string'},
-                    'expected_return_value': {'type': 'number'},
-                },
-                'required': ['id', 'name', 'entry'],
-            },
+    'type': 'array',
+    'minItems': 1,
+    'items': {
+        'type': 'object',
+        'properties': {
+            'id': {'type': 'string'},
+            'name': {'type': 'string'},
+            'description': {'type': 'string'},
+            'entry': {'type': 'string'},
+            'language': {'type': 'string'},
+            'expected_return_value': {'type': 'number'},
         },
+        'required': ['id', 'name', 'entry'],
     },
-    'required': ['hooks'],
 }
 
 
-def check_is_valid_manifest(file_contents):
-    file_objects = yaml.load(file_contents)
-
-    jsonschema.validate(file_objects, MANIFEST_JSON_SCHEMA)
-
-    for hook_config in file_objects['hooks']:
+def additional_manifest_check(obj):
+    for hook_config in obj:
         language = hook_config.get('language')
 
         if language is not None and not any(
@@ -57,41 +44,33 @@ def check_is_valid_manifest(file_contents):
             )
 
 
+validate_manifest = get_validator(
+    C.MANIFEST_FILE,
+    MANIFEST_JSON_SCHEMA,
+    InvalidManifestError,
+    additional_manifest_check,
+)
+
+
 def run(argv):
     parser = argparse.ArgumentParser()
     parser.add_argument(
-        '--filename',
-        required=False, default=None,
+        'filename',
+        nargs='?', default=None,
         help='Manifest filename.  Defaults to {0} at root of git repo'.format(
             C.MANIFEST_FILE,
         )
     )
     args = parser.parse_args(argv)
 
-    if args.filename is None:
-        # TODO: filename = git.get_root() + C.MANIFEST_FILE
-        raise NotImplementedError
-    else:
-        filename = args.filename
-
-    if not os.path.exists(filename):
-        print('File {0} does not exist'.format(filename))
-        return 1
-
-    file_contents = open(filename, 'r').read()
-
     try:
-        yaml.load(file_contents)
-    except Exception as e:
-        print('File {0} is not a valid yaml file'.format(filename))
-        print(str(e))
-        return 1
-
-    try:
-        check_is_valid_manifest(file_contents)
-    except (jsonschema.exceptions.ValidationError, InvalidManifestError) as e:
-        print('File {0} is not a valid manifest file'.format(filename))
-        print(str(e))
+        validate_manifest(args.filename)
+    except InvalidManifestError as e:
+        print(e.args[0])
+        # If we have more than one exception argument print the stringified
+        # version
+        if len(e.args) > 1:
+            print(str(e.args[1]))
         return 1
 
     return 0

pre_commit/constants.py

@@ -1,5 +1,5 @@
 
-PRE_COMMIT_FILE = '.pre-commit-config.yaml'
+CONFIG_FILE = '.pre-commit-config.yaml'
 
 PRE_COMMIT_DIR = '.pre-commit-files'
 

pre_commit/git.py

@@ -54,7 +54,8 @@ class PreCommitProject(object):
         with self.in_checkout():
             if local.path('setup.py').exists():
                 local['virtualenv']['py_env']()
-                local['bash'][local['pip']['install', '.']]
+                local['bash']['-c', 'source py_env/bin/activate && pip install .']()
+                print local.cwd.getpath()
 
 def create_repo_in_env(git_repo_path, sha):
     project = PreCommitProject(git_repo_path, sha)

tests/clientlib/validate_base_test.py

@@ -0,0 +1,67 @@
+
+import __builtin__
+
+import os.path
+import mock
+import pytest
+
+from pre_commit import git
+from pre_commit.clientlib.validate_base import get_validator
+
+
+class AdditionalValidatorError(ValueError): pass
+
+
+@pytest.fixture
+def noop_validator():
+    return get_validator('example_manifest.yaml', {}, ValueError)
+
+
+@pytest.fixture
+def array_validator():
+    return get_validator('', {'type': 'array'}, ValueError)
+
+
+@pytest.fixture
+def additional_validator():
+    def raises_always(obj):
+        raise AdditionalValidatorError
+
+    return get_validator(
+        'example_manifest.yaml',
+        {},
+        ValueError,
+        additional_validation_strategy=raises_always,
+    )
+
+
+def test_raises_for_non_existent_file(noop_validator):
+    with pytest.raises(ValueError):
+        noop_validator('file_that_does_not_exist.yaml')
+
+
+def test_raises_for_invalid_yaml_file(noop_validator):
+    with pytest.raises(ValueError):
+        noop_validator('tests/data/non_parseable_yaml_file.yaml')
+
+
+def test_defaults_to_backup_filename(noop_validator):
+    with mock.patch.object(__builtin__, 'open', side_effect=open) as mock_open:
+        noop_validator()
+        mock_open.assert_called_once_with(
+            os.path.join(git.get_root(), 'example_manifest.yaml'), 'r',
+        )
+
+
+def test_raises_for_failing_schema(array_validator):
+    with pytest.raises(ValueError):
+        array_validator('tests/data/valid_yaml_but_invalid_manifest.yaml')
+
+
+def test_passes_array_schema(array_validator):
+    array_validator('tests/data/array_yaml_file.yaml')
+
+
+def test_raises_when_additional_validation_fails(additional_validator):
+    with pytest.raises(AdditionalValidatorError):
+        additional_validator()

tests/clientlib/validate_manifest_test.py

@@ -1,86 +1,62 @@
 
-import __builtin__
 import jsonschema
+import jsonschema.exceptions
 import pytest
-import mock
 
-from pre_commit.clientlib.validate_manifest import check_is_valid_manifest
+from pre_commit.clientlib.validate_manifest import additional_manifest_check
 from pre_commit.clientlib.validate_manifest import InvalidManifestError
+from pre_commit.clientlib.validate_manifest import MANIFEST_JSON_SCHEMA
 from pre_commit.clientlib.validate_manifest import run
 
 
-@pytest.yield_fixture
-def print_mock():
-    with mock.patch.object(__builtin__, 'print', autospec=True) as print_mock_obj:
-        yield print_mock_obj
-
-
-def test_run_returns_1_for_non_existent_module(print_mock):
-    non_existent_filename = 'file_that_does_not_exist'
-    ret = run(['--filename', non_existent_filename])
-    assert ret == 1
-    print_mock.assert_called_once_with(
-        'File {0} does not exist'.format(non_existent_filename),
-    )
-
-
-def test_run_returns_1_for_non_yaml_file(print_mock):
-    non_parseable_filename = 'tests/data/non_parseable_yaml_file.yaml'
-    ret = run(['--filename', non_parseable_filename])
-    assert ret == 1
-    print_mock.assert_any_call(
-        'File {0} is not a valid yaml file'.format(non_parseable_filename),
-    )
-
-
-def test_returns_1_for_valid_yaml_file_but_invalid_manifest(print_mock):
-    invalid_manifest = 'tests/data/valid_yaml_but_invalid_manifest.yaml'
-    ret = run(['--filename', invalid_manifest])
-    assert ret == 1
-    print_mock.assert_any_call(
-        'File {0} is not a valid manifest file'.format(invalid_manifest)
-    )
-
-
 def test_returns_0_for_valid_manifest():
-    valid_manifest = 'example_manifest.yaml'
-    ret = run(['--filename', valid_manifest])
-    assert ret == 0
+    assert run(['example_manifest.yaml']) == 0
 
 
-@pytest.mark.parametrize(('manifest', 'expected_exception_type'), (
+def test_returns_0_for_our_manifest():
+    assert run([]) == 0
+
+
+def test_returns_1_for_failing():
+    assert run(['tests/data/valid_yaml_but_invalid_manifest.yaml']) == 1
+
+
+def test_additional_manifest_check_raises_for_bad_language():
+    with pytest.raises(InvalidManifestError):
+        additional_manifest_check([{'id': 'foo', 'language': 'not valid'}])
+
+
+@pytest.mark.parametrize(('obj'), (
+    [{}],
+    [{'language': 'python'}],
+    [{'language': 'python>2.6'}],
+))
+def test_additional_manifest_check_is_ok_with_missing_language(obj):
+    additional_manifest_check(obj)
+
+
+def is_valid_according_to_schema(obj, schema):
+    try:
+        jsonschema.validate(obj, schema)
+        return True
+    except jsonschema.exceptions.ValidationError:
+        return False
+
+
+@pytest.mark.parametrize(('manifest_obj', 'expected'), (
+    ([], False),
+    ([{'id': 'a', 'name': 'b', 'entry': 'c'}], True),
     (
-        """
-hooks:
-    -
-        id: foo
-        entry: foo
-        """,
-        jsonschema.exceptions.ValidationError,
-    ),
-    (
-        """
-hooks:
-    -
-        id: foo
-        name: Foo
-        language: Not a Language lol
-        entry: foo
-        """,
-        InvalidManifestError,
+        [{
+             'id': 'a',
+             'name': 'b',
+             'entry': 'c',
+             'language': 'python',
+             'expected_return_value': 0,
+        }],
+        True,
     ),
 ))
-def test_check_invalid_manifests(manifest, expected_exception_type):
-    with pytest.raises(expected_exception_type):
-        check_is_valid_manifest(manifest)
-
-
-def test_valid_manifest_is_valid():
-    check_is_valid_manifest("""
-hooks:
-    -
-        id: foo
-        name: Foo
-        entry: foo
-        language: python>2.6
-    """)
+def test_is_valid_according_to_schema(manifest_obj, expected):
+    ret = is_valid_according_to_schema(manifest_obj, MANIFEST_JSON_SCHEMA)
+    assert ret is expected

tests/conftest.py

@@ -0,0 +1,10 @@
+
+import pytest
+from plumbum import local
+
+
+@pytest.yield_fixture
+def empty_git_dir(tmpdir):
+    with local.cwd(tmpdir.strpath):
+        local['git']['init']()
+        yield tmpdir.strpath

tests/data/array_yaml_file.yaml

@@ -0,0 +1,2 @@
+- foo
+- bar

tests/git_test.py

@@ -16,14 +16,6 @@ def get_sha(git_repo):
     with local.cwd(git_repo):
         return (local['git']['log', '--format="%H"'] | local['head']['-n1'])().strip('"\n')
 
-
-@pytest.yield_fixture
-def empty_git_dir(tmpdir):
-    with local.cwd(tmpdir.strpath):
-        local['git']['init']()
-        yield tmpdir.strpath
-
-
 @pytest.yield_fixture
 def dummy_git_repo(empty_git_dir):
     local['touch']['dummy']()