From 96174deac671b451ee2a3fbdc647ad9606415e15 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Thu, 5 Jun 2014 18:37:33 -0700 Subject: [PATCH 1/5] Make hooks specify files. Optionally allow config to override manifest. --- .pre-commit-config.yaml | 2 +- example_hooks.yaml | 5 ++- example_pre-commit-config.yaml | 3 -- hooks.yaml | 12 +++--- pre_commit/clientlib/validate_config.py | 4 +- pre_commit/clientlib/validate_manifest.py | 3 +- pre_commit/repository.py | 2 +- .../consumer_repo/.pre-commit-config.yaml | 7 ---- .../resources/failing_hook_repo/hooks.yaml | 1 + testing/resources/manifest_without_foo.yaml | 1 + .../node_0_11_8_hooks_repo/hooks.yaml | 1 + testing/resources/node_hooks_repo/hooks.yaml | 1 + testing/resources/prints_cwd_repo/hooks.yaml | 1 + .../resources/python3_hooks_repo/hooks.yaml | 1 + .../resources/python_hooks_repo/hooks.yaml | 1 + .../ruby_1_9_3_p547_hooks_repo/hooks.yaml | 1 + testing/resources/ruby_hooks_repo/hooks.yaml | 1 + .../resources/script_hooks_repo/hooks.yaml | 1 + .../system_hook_with_spaces_repo/hooks.yaml | 1 + .../valid_yaml_but_invalid_config.yaml | 1 - tests/clientlib/validate_manifest_test.py | 40 +++++++++++++------ tests/commands_test.py | 4 +- tests/conftest.py | 24 +++++------ tests/manifest_test.py | 2 + tests/repository_test.py | 11 +++++ 25 files changed, 81 insertions(+), 50 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index f84b5d84..edfb2dcd 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,5 +1,5 @@ - repo: git@github.com:pre-commit/pre-commit-hooks - sha: 59f23b7c556ce1cf66eb6dc574e10d2b4274be4b + sha: 7c003425b35fff516c0ee88f4040c8c208d474bd hooks: - id: trailing-whitespace files: \.(js|rb|md|py|sh|txt|yaml|yml)$ diff --git a/example_hooks.yaml b/example_hooks.yaml index cb341036..4706cfe8 100644 --- a/example_hooks.yaml +++ b/example_hooks.yaml @@ -15,12 +15,13 @@ - id: my_hook name: My Simple Hook description: This is my simple hook that does blah - entry: my-simple-hook.py + entry: my-simple-hook language: python - expected_return_value: 0 + files: \.py$ - 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 language: script + files: \.(py|sh)$ expected_return_value: 1 diff --git a/example_pre-commit-config.yaml b/example_pre-commit-config.yaml index bfaea849..cb72d885 100644 --- a/example_pre-commit-config.yaml +++ b/example_pre-commit-config.yaml @@ -2,8 +2,5 @@ sha: cd74dc150c142c3be70b24eaf0b02cae9d235f37 hooks: - id: pyflakes - files: '\.py$' - id: jslint - files: '\.js$' - id: trim_trailing_whitespace - files: '\.py$' diff --git a/hooks.yaml b/hooks.yaml index 062e09c5..d638cf81 100644 --- a/hooks.yaml +++ b/hooks.yaml @@ -1,10 +1,12 @@ -- id: validate_manifest - name: Validate Pre-Commit Manifest - description: This validator validates a pre-commit hooks manifest file - entry: validate-manifest - language: python - id: validate_config name: Validate Pre-Commit Config description: This validator validates a pre-commit hooks config file entry: validate-config language: python + files: ^\.pre-commit-config.yaml$ +- id: validate_manifest + name: Validate Pre-Commit Manifest + description: This validator validates a pre-commit hooks manifest file + entry: validate-manifest + language: python + files: ^hooks.yaml$ diff --git a/pre_commit/clientlib/validate_config.py b/pre_commit/clientlib/validate_config.py index 53d2e81b..d9f97698 100644 --- a/pre_commit/clientlib/validate_config.py +++ b/pre_commit/clientlib/validate_config.py @@ -32,7 +32,7 @@ CONFIG_JSON_SCHEMA = { 'items': {'type': 'string'}, }, }, - 'required': ['id', 'files'], + 'required': ['id'], } } }, @@ -55,7 +55,7 @@ def try_regex(repo, hook, value, field_name): def validate_config_extra(config): for repo in config: for hook in repo['hooks']: - try_regex(repo, hook['id'], hook['files'], 'files') + try_regex(repo, hook['id'], hook.get('files', ''), 'files') try_regex(repo, hook['id'], hook['exclude'], 'exclude') diff --git a/pre_commit/clientlib/validate_manifest.py b/pre_commit/clientlib/validate_manifest.py index 61d3324e..72e8a53f 100644 --- a/pre_commit/clientlib/validate_manifest.py +++ b/pre_commit/clientlib/validate_manifest.py @@ -21,9 +21,10 @@ MANIFEST_JSON_SCHEMA = { 'entry': {'type': 'string'}, 'language': {'type': 'string'}, 'language_version': {'type': 'string', 'default': 'default'}, + 'files': {'type': 'string'}, 'expected_return_value': {'type': 'number', 'default': 0}, }, - 'required': ['id', 'name', 'entry', 'language'], + 'required': ['id', 'name', 'entry', 'language', 'files'], }, } diff --git a/pre_commit/repository.py b/pre_commit/repository.py index 7f762d31..5799efe6 100644 --- a/pre_commit/repository.py +++ b/pre_commit/repository.py @@ -38,7 +38,7 @@ class Repository(object): def hooks(self): # TODO: merging in manifest dicts is a smell imo return OrderedDict( - (hook['id'], dict(hook, **self.manifest.hooks[hook['id']])) + (hook['id'], dict(self.manifest.hooks[hook['id']], **hook)) for hook in self.repo_config['hooks'] ) diff --git a/testing/resources/consumer_repo/.pre-commit-config.yaml b/testing/resources/consumer_repo/.pre-commit-config.yaml index 1bffc6a9..0f9b5642 100644 --- a/testing/resources/consumer_repo/.pre-commit-config.yaml +++ b/testing/resources/consumer_repo/.pre-commit-config.yaml @@ -2,19 +2,12 @@ sha: bec87f6c87284ea15dbcf7801810404c8036bab4 hooks: - id: pyflakes - files: \.py$ - id: debug-statements - files: \.py$ - id: trailing-whitespace - files: \.(py|sh|yaml)$ - id: name-tests-test - files: tests/.+\.py$ - id: end-of-file-fixer - files: \.(py|sh|yaml)$ - repo: git@github.com:pre-commit/pre-commit sha: c62c1a3b513ab9e057e85a5e950bd7c438371076 hooks: - id: validate_manifest - files: ^hooks.yaml$ - id: validate_config - files: ^\.pre-commit-config.yaml$ diff --git a/testing/resources/failing_hook_repo/hooks.yaml b/testing/resources/failing_hook_repo/hooks.yaml index ecdf9a00..118cc8b1 100644 --- a/testing/resources/failing_hook_repo/hooks.yaml +++ b/testing/resources/failing_hook_repo/hooks.yaml @@ -2,3 +2,4 @@ name: Failing hook entry: bin/hook.sh language: script + files: . diff --git a/testing/resources/manifest_without_foo.yaml b/testing/resources/manifest_without_foo.yaml index 220eedeb..0220233a 100644 --- a/testing/resources/manifest_without_foo.yaml +++ b/testing/resources/manifest_without_foo.yaml @@ -2,3 +2,4 @@ name: Bar entry: bar language: python + files: \.py$ diff --git a/testing/resources/node_0_11_8_hooks_repo/hooks.yaml b/testing/resources/node_0_11_8_hooks_repo/hooks.yaml index 6fc04785..005a1e3b 100644 --- a/testing/resources/node_0_11_8_hooks_repo/hooks.yaml +++ b/testing/resources/node_0_11_8_hooks_repo/hooks.yaml @@ -3,3 +3,4 @@ entry: node-11-8-hook language: node language_version: 0.11.8 + files: \.js$ diff --git a/testing/resources/node_hooks_repo/hooks.yaml b/testing/resources/node_hooks_repo/hooks.yaml index 2088caa1..257698a4 100644 --- a/testing/resources/node_hooks_repo/hooks.yaml +++ b/testing/resources/node_hooks_repo/hooks.yaml @@ -2,3 +2,4 @@ name: Foo entry: foo language: node + files: \.js$ diff --git a/testing/resources/prints_cwd_repo/hooks.yaml b/testing/resources/prints_cwd_repo/hooks.yaml index e743dda0..70923792 100644 --- a/testing/resources/prints_cwd_repo/hooks.yaml +++ b/testing/resources/prints_cwd_repo/hooks.yaml @@ -2,3 +2,4 @@ name: Prints Cwd entry: pwd language: system + files: \.sh$ diff --git a/testing/resources/python3_hooks_repo/hooks.yaml b/testing/resources/python3_hooks_repo/hooks.yaml index 50c0ee9e..becc2837 100644 --- a/testing/resources/python3_hooks_repo/hooks.yaml +++ b/testing/resources/python3_hooks_repo/hooks.yaml @@ -3,3 +3,4 @@ entry: python3-hook language: python language_version: python3.3 + files: \.py$ diff --git a/testing/resources/python_hooks_repo/hooks.yaml b/testing/resources/python_hooks_repo/hooks.yaml index 661835e7..e10ad50f 100644 --- a/testing/resources/python_hooks_repo/hooks.yaml +++ b/testing/resources/python_hooks_repo/hooks.yaml @@ -2,3 +2,4 @@ name: Foo entry: foo language: python + files: \.py$ diff --git a/testing/resources/ruby_1_9_3_p547_hooks_repo/hooks.yaml b/testing/resources/ruby_1_9_3_p547_hooks_repo/hooks.yaml index a55460d2..9f06a970 100644 --- a/testing/resources/ruby_1_9_3_p547_hooks_repo/hooks.yaml +++ b/testing/resources/ruby_1_9_3_p547_hooks_repo/hooks.yaml @@ -3,3 +3,4 @@ entry: ruby_hook language: ruby language_version: 1.9.3-p547 + files: \.rb$ diff --git a/testing/resources/ruby_hooks_repo/hooks.yaml b/testing/resources/ruby_hooks_repo/hooks.yaml index 2496e5e4..aa15872f 100644 --- a/testing/resources/ruby_hooks_repo/hooks.yaml +++ b/testing/resources/ruby_hooks_repo/hooks.yaml @@ -2,3 +2,4 @@ name: Ruby Hook entry: ruby_hook language: ruby + files: \.rb$ diff --git a/testing/resources/script_hooks_repo/hooks.yaml b/testing/resources/script_hooks_repo/hooks.yaml index 6f9c0faa..21cad4a3 100644 --- a/testing/resources/script_hooks_repo/hooks.yaml +++ b/testing/resources/script_hooks_repo/hooks.yaml @@ -2,3 +2,4 @@ name: Bash hook entry: bin/hook.sh language: script + files: '' diff --git a/testing/resources/system_hook_with_spaces_repo/hooks.yaml b/testing/resources/system_hook_with_spaces_repo/hooks.yaml index 366f44bd..dda8fb96 100644 --- a/testing/resources/system_hook_with_spaces_repo/hooks.yaml +++ b/testing/resources/system_hook_with_spaces_repo/hooks.yaml @@ -2,3 +2,4 @@ name: System hook with spaces entry: /usr/bin/python -c 'import sys; print("Hello World")' language: system + files: \.sh$ diff --git a/testing/resources/valid_yaml_but_invalid_config.yaml b/testing/resources/valid_yaml_but_invalid_config.yaml index 81ccd6a6..2ed187b2 100644 --- a/testing/resources/valid_yaml_but_invalid_config.yaml +++ b/testing/resources/valid_yaml_but_invalid_config.yaml @@ -3,4 +3,3 @@ - id: pyflakes - id: jslint - id: trim_trailing_whitespace - files: '*.py' diff --git a/tests/clientlib/validate_manifest_test.py b/tests/clientlib/validate_manifest_test.py index de8e01ac..800d292b 100644 --- a/tests/clientlib/validate_manifest_test.py +++ b/tests/clientlib/validate_manifest_test.py @@ -46,20 +46,34 @@ def test_additional_manifest_check_languages_failing(obj): additional_manifest_check(obj) -@pytest.mark.parametrize(('manifest_obj', 'expected'), ( - ([], False), - ([{'id': 'a', 'name': 'b', 'entry': 'c', 'language': 'python'}], True), +@pytest.mark.parametrize( + ('manifest_obj', 'expected'), ( - [{ - 'id': 'a', - 'name': 'b', - 'entry': 'c', - 'language': 'python', - 'expected_return_value': 0, - }], - True, - ), -)) + ([], False), + ( + [{ + 'id': 'a', + 'name': 'b', + 'entry': 'c', + 'language': 'python', + 'files': r'\.py$' + }], + True, + ), + ( + [{ + 'id': 'a', + 'name': 'b', + 'entry': 'c', + 'language': 'python', + 'language_version': 'python3.3', + 'files': r'\.py$', + 'expected_return_value': 0, + }], + True, + ), + ) +) 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 diff --git a/tests/commands_test.py b/tests/commands_test.py index acd98ebf..af1f1b08 100644 --- a/tests/commands_test.py +++ b/tests/commands_test.py @@ -58,7 +58,7 @@ def up_to_date_repo(python_hooks_repo): config = OrderedDict(( ('repo', python_hooks_repo), ('sha', get_head_sha(python_hooks_repo)), - ('hooks', [OrderedDict((('id', 'foo'), ('files', '')))]), + ('hooks', [OrderedDict((('id', 'foo'),))]), )) wrapped_config = apply_defaults([config], CONFIG_JSON_SCHEMA) validate_config_extra(wrapped_config) @@ -147,7 +147,7 @@ def hook_disappearing_repo(python_hooks_repo): config = OrderedDict(( ('repo', python_hooks_repo), ('sha', get_head_sha(python_hooks_repo)), - ('hooks', [OrderedDict((('id', 'foo'), ('files', '')))]), + ('hooks', [OrderedDict((('id', 'foo'),))]), )) config_wrapped = apply_defaults([config], CONFIG_JSON_SCHEMA) validate_config_extra(config_wrapped) diff --git a/tests/conftest.py b/tests/conftest.py index bcb222bd..c70d4f6e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -122,11 +122,11 @@ def system_hook_with_spaces_repo(dummy_git_repo): yield _make_repo(dummy_git_repo, 'system_hook_with_spaces_repo') -def _make_config(path, hook_id, file_regex): +def _make_config(path, hook_id): config = { 'repo': path, 'sha': get_head_sha(path), - 'hooks': [{'id': hook_id, 'files': file_regex}], + 'hooks': [{'id': hook_id}], } config_wrapped = apply_defaults([config], CONFIG_JSON_SCHEMA) validate_config_extra(config_wrapped) @@ -135,48 +135,48 @@ def _make_config(path, hook_id, file_regex): @pytest.yield_fixture def config_for_node_hooks_repo(node_hooks_repo): - yield _make_config(node_hooks_repo, 'foo', '\\.js$') + yield _make_config(node_hooks_repo, 'foo') @pytest.yield_fixture def config_for_node_0_11_8_hooks_repo(node_0_11_8_hooks_repo): - yield _make_config(node_0_11_8_hooks_repo, 'node-11-8-hook', '\\.js$') + yield _make_config(node_0_11_8_hooks_repo, 'node-11-8-hook') @pytest.yield_fixture def config_for_ruby_hooks_repo(ruby_hooks_repo): - yield _make_config(ruby_hooks_repo, 'ruby_hook', '\\.rb$') + yield _make_config(ruby_hooks_repo, 'ruby_hook') @pytest.yield_fixture def config_for_ruby_1_9_3_p547_hooks_repo(ruby_1_9_3_p547_hooks_repo): - yield _make_config(ruby_1_9_3_p547_hooks_repo, 'ruby_hook', '\\.rb$') + yield _make_config(ruby_1_9_3_p547_hooks_repo, 'ruby_hook') @pytest.yield_fixture def config_for_python_hooks_repo(python_hooks_repo): - yield _make_config(python_hooks_repo, 'foo', '\\.py$') + yield _make_config(python_hooks_repo, 'foo') @pytest.yield_fixture def config_for_python3_hooks_repo(python3_hooks_repo): - yield _make_config(python3_hooks_repo, 'python3-hook', '\\.py$') + yield _make_config(python3_hooks_repo, 'python3-hook') @pytest.yield_fixture def config_for_prints_cwd_repo(prints_cwd_repo): - yield _make_config(prints_cwd_repo, 'prints_cwd', '^$') + yield _make_config(prints_cwd_repo, 'prints_cwd') @pytest.yield_fixture def config_for_script_hooks_repo(script_hooks_repo): - yield _make_config(script_hooks_repo, 'bash_hook', '') + yield _make_config(script_hooks_repo, 'bash_hook') @pytest.yield_fixture def config_for_system_hook_with_spaces(system_hook_with_spaces_repo): yield _make_config( - system_hook_with_spaces_repo, 'system-hook-with-spaces', '', + system_hook_with_spaces_repo, 'system-hook-with-spaces', ) @@ -198,7 +198,7 @@ def repo_with_passing_hook(config_for_script_hooks_repo, empty_git_dir): @pytest.yield_fixture def repo_with_failing_hook(failing_hook_repo, empty_git_dir): - _make_repo_from_configs(_make_config(failing_hook_repo, 'failing_hook', '')) + _make_repo_from_configs(_make_config(failing_hook_repo, 'failing_hook')) yield empty_git_dir diff --git a/tests/manifest_test.py b/tests/manifest_test.py index c742b61f..e419ce7a 100644 --- a/tests/manifest_test.py +++ b/tests/manifest_test.py @@ -17,6 +17,7 @@ def test_manifest_contents(manifest): 'description': '', 'entry': 'bin/hook.sh', 'expected_return_value': 0, + 'files': '', 'id': 'bash_hook', 'language': 'script', 'language_version': 'default', @@ -29,6 +30,7 @@ def test_hooks(manifest): 'description': '', 'entry': 'bin/hook.sh', 'expected_return_value': 0, + 'files': '', 'id': 'bash_hook', 'language': 'script', 'language_version': 'default', diff --git a/tests/repository_test.py b/tests/repository_test.py index 33d732d1..8f0669b4 100644 --- a/tests/repository_test.py +++ b/tests/repository_test.py @@ -32,6 +32,7 @@ def test_run_versioned_hook(config_for_python3_hooks_repo, store): assert ret[1] == "3.3\n['/dev/null']\nHello World\n" +@skipif_slowtests_false @pytest.mark.integration def test_run_versioned_node_hook(config_for_node_0_11_8_hooks_repo, store): repo = Repository.create(config_for_node_0_11_8_hooks_repo, store) @@ -152,3 +153,13 @@ def test_really_long_file_paths(config_for_python_hooks_repo, store): with local.cwd(path): repo = Repository.create(config_for_python_hooks_repo, store) repo.require_installed() + + +@pytest.mark.integration +def test_config_overrides_repo_specifics(config_for_script_hooks_repo, store): + repo = Repository.create(config_for_script_hooks_repo, store) + assert repo.hooks['bash_hook']['files'] == '' + # Set the file regex to something else + config_for_script_hooks_repo['hooks'][0]['files'] = '\\.sh$' + repo = Repository.create(config_for_script_hooks_repo, store) + assert repo.hooks['bash_hook']['files'] == '\\.sh$' From bd1afceeaf7f5c0518337f6fc70da637d16612a6 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Thu, 5 Jun 2014 18:38:35 -0700 Subject: [PATCH 2/5] Update config to point at new sha. --- .pre-commit-config.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index edfb2dcd..4163fac9 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -16,7 +16,7 @@ args: - --max-line-length=131 - repo: git@github.com:pre-commit/pre-commit - sha: c7cfed699245191e6b0d43d3890fd582157e3919 + sha: 96174deac671b451ee2a3fbdc647ad9606415e15 hooks: - id: validate_config files: ^\.pre-commit-config.yaml$ From bf912cfebb8fdd3d16bb9335e93f7c9dada949f8 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Fri, 6 Jun 2014 06:53:00 -0700 Subject: [PATCH 3/5] Add checking for manifest files regex. --- pre_commit/clientlib/validate_base.py | 9 ++++++ pre_commit/clientlib/validate_config.py | 6 ++-- pre_commit/clientlib/validate_manifest.py | 34 ++++++++++++++++------- tests/clientlib/validate_manifest_test.py | 15 ++++++---- 4 files changed, 45 insertions(+), 19 deletions(-) diff --git a/pre_commit/clientlib/validate_base.py b/pre_commit/clientlib/validate_base.py index 1b8856aa..27f363a9 100644 --- a/pre_commit/clientlib/validate_base.py +++ b/pre_commit/clientlib/validate_base.py @@ -4,12 +4,21 @@ import argparse import jsonschema import jsonschema.exceptions import os.path +import re import yaml from pre_commit.jsonschema_extensions import apply_defaults from pre_commit.util import entry +def is_regex_valid(regex): + try: + re.compile(regex) + return True + except re.error: + return False + + def get_validator( json_schema, exception_type, diff --git a/pre_commit/clientlib/validate_config.py b/pre_commit/clientlib/validate_config.py index d9f97698..997c84fe 100644 --- a/pre_commit/clientlib/validate_config.py +++ b/pre_commit/clientlib/validate_config.py @@ -1,8 +1,8 @@ -import re import sys from pre_commit.clientlib.validate_base import get_run_function from pre_commit.clientlib.validate_base import get_validator +from pre_commit.clientlib.validate_base import is_regex_valid class InvalidConfigError(ValueError): @@ -42,9 +42,7 @@ CONFIG_JSON_SCHEMA = { def try_regex(repo, hook, value, field_name): - try: - re.compile(value) - except re.error: + if not is_regex_valid(value): raise InvalidConfigError( 'Invalid {0} regex at {1}, {2}: {3}'.format( field_name, repo, hook, value, diff --git a/pre_commit/clientlib/validate_manifest.py b/pre_commit/clientlib/validate_manifest.py index 72e8a53f..fed5a033 100644 --- a/pre_commit/clientlib/validate_manifest.py +++ b/pre_commit/clientlib/validate_manifest.py @@ -2,6 +2,7 @@ import sys from pre_commit.clientlib.validate_base import get_run_function from pre_commit.clientlib.validate_base import get_validator +from pre_commit.clientlib.validate_base import is_regex_valid from pre_commit.languages.all import all_languages @@ -29,18 +30,31 @@ MANIFEST_JSON_SCHEMA = { } +def validate_languages(hook_config): + if hook_config['language'] not in all_languages: + raise InvalidManifestError( + 'Expected language {0} for {1} to be one of {2!r}'.format( + hook_config['id'], + hook_config['language'], + all_languages, + ) + ) + + +def validate_files(hook_config): + if not is_regex_valid(hook_config['files']): + raise InvalidManifestError( + 'Invalid files regex at {0}: {1}'.format( + hook_config['id'], + hook_config['files'], + ) + ) + + def additional_manifest_check(obj): for hook_config in obj: - language = hook_config['language'] - - if language not in all_languages: - raise InvalidManifestError( - 'Expected language {0} for {1} to be one of {2!r}'.format( - hook_config['id'], - hook_config['language'], - all_languages, - ) - ) + validate_languages(hook_config) + validate_files(hook_config) load_manifest = get_validator( diff --git a/tests/clientlib/validate_manifest_test.py b/tests/clientlib/validate_manifest_test.py index 800d292b..5dba2747 100644 --- a/tests/clientlib/validate_manifest_test.py +++ b/tests/clientlib/validate_manifest_test.py @@ -28,20 +28,25 @@ def test_additional_manifest_check_raises_for_bad_language(): @pytest.mark.parametrize( - 'obj', ([{'language': 'python'}], [{'language': 'ruby'}]), + 'obj', + ( + [{'language': 'python', 'files': ''}], + [{'language': 'ruby', 'files': ''}] + ), ) -def test_additional_manifest_check_languages(obj): +def test_additional_manifest_check_passing(obj): additional_manifest_check(obj) @pytest.mark.parametrize( 'obj', ( - [{'id': 'a', 'language': 'not a language'}], - [{'id': 'a', 'language': 'python3'}], + [{'id': 'a', 'language': 'not a language', 'files': ''}], + [{'id': 'a', 'language': 'python3', 'files': ''}], + [{'id': 'a', 'language': 'python', 'files': 'invalid regex('}], ), ) -def test_additional_manifest_check_languages_failing(obj): +def test_additional_manifest_failing(obj): with pytest.raises(InvalidManifestError): additional_manifest_check(obj) From 01b557c497c7ff6727b9a6659fdf65f4fc00cfff Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Fri, 6 Jun 2014 07:38:25 -0700 Subject: [PATCH 4/5] Various cleanup. --- .pre-commit-config.yaml | 10 ----- pre_commit/commands.py | 4 +- pre_commit/git.py | 4 +- pre_commit/languages/all.py | 7 +++- pre_commit/output.py | 4 +- pre_commit/prefixed_command_runner.py | 7 +++- pre_commit/run.py | 8 +++- setup.py | 5 ++- tests/clientlib/validate_base_test.py | 4 +- tests/commands_test.py | 58 ++++++++++++++++++++++----- tests/prefixed_command_runner_test.py | 8 +++- tox.ini | 3 -- 12 files changed, 85 insertions(+), 37 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 4163fac9..5887105a 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -2,23 +2,13 @@ sha: 7c003425b35fff516c0ee88f4040c8c208d474bd hooks: - id: trailing-whitespace - files: \.(js|rb|md|py|sh|txt|yaml|yml)$ - id: end-of-file-fixer - files: \.(js|rb|md|py|sh|txt|yaml|yml)$ - id: check-yaml - files: \.(yaml|yml)$ - id: debug-statements - files: \.py$ - id: name-tests-test - files: tests/.+\.py$ - id: flake8 - files: \.py$ - args: - - --max-line-length=131 - repo: git@github.com:pre-commit/pre-commit sha: 96174deac671b451ee2a3fbdc647ad9606415e15 hooks: - id: validate_config - files: ^\.pre-commit-config.yaml$ - id: validate_manifest - files: ^hooks.yaml$ diff --git a/pre_commit/commands.py b/pre_commit/commands.py index 4ff8d01e..a25e7830 100644 --- a/pre_commit/commands.py +++ b/pre_commit/commands.py @@ -29,7 +29,9 @@ logger = logging.getLogger('pre_commit') def install(runner): """Install the pre-commit hooks.""" - pre_commit_file = pkg_resources.resource_filename('pre_commit', 'resources/pre-commit.sh') + pre_commit_file = pkg_resources.resource_filename( + 'pre_commit', 'resources/pre-commit.sh', + ) with open(runner.pre_commit_path, 'w') as pre_commit_file_obj: pre_commit_file_obj.write(open(pre_commit_file).read()) diff --git a/pre_commit/git.py b/pre_commit/git.py index 847aedf4..08db8440 100644 --- a/pre_commit/git.py +++ b/pre_commit/git.py @@ -29,7 +29,9 @@ def is_in_merge_conflict(): def parse_merge_msg_for_conflicts(merge_msg): # Conflicted files start with tabs return [ - line.strip() for line in merge_msg.splitlines() if line.startswith('\t') + line.strip() + for line in merge_msg.splitlines() + if line.startswith('\t') ] diff --git a/pre_commit/languages/all.py b/pre_commit/languages/all.py index 27565e42..50d83af6 100644 --- a/pre_commit/languages/all.py +++ b/pre_commit/languages/all.py @@ -9,16 +9,19 @@ from pre_commit.languages import system # # Use None for no environment # ENVIRONMENT_DIR = 'foo_env' # -# def install_environment(repo_cmd_runner): +# def install_environment(repo_cmd_runner, version='default'): # """Installs a repository in the given repository. Note that the current # working directory will already be inside the repository. # # Args: # repo_cmd_runner - `PrefixedCommandRunner` bound to the repository. +# version - A version specified in the hook configuration or +# 'default'. # """ # # def run_hook(repo_cmd_runner, hook, file_args): -# """Runs a hook and returns the returncode and output of running that hook. +# """Runs a hook and returns the returncode and output of running that +# hook. # # Args: # repo_cmd_runner - `PrefixedCommandRunner` bound to the repository. diff --git a/pre_commit/output.py b/pre_commit/output.py index a4d99dd3..e3078391 100644 --- a/pre_commit/output.py +++ b/pre_commit/output.py @@ -26,8 +26,8 @@ def get_hook_message( >>> print_hook_message('start', end_len=6) start............................................................... - # Print `start` followed by dots with the end message colored if coloring is - # specified and a newline afterwards + # Print `start` followed by dots with the end message colored if coloring + # is specified and a newline afterwards >>> print_hook_message( 'start', end_msg='end', diff --git a/pre_commit/prefixed_command_runner.py b/pre_commit/prefixed_command_runner.py index 9c3c6b39..90678ddb 100644 --- a/pre_commit/prefixed_command_runner.py +++ b/pre_commit/prefixed_command_runner.py @@ -42,7 +42,12 @@ class PrefixedCommandRunner(object): will run ['/tmp/foo/foo.sh', 'bar', 'baz'] """ - def __init__(self, prefix_dir, popen=subprocess.Popen, makedirs=os.makedirs): + def __init__( + self, + prefix_dir, + popen=subprocess.Popen, + makedirs=os.makedirs + ): self.prefix_dir = prefix_dir.rstrip(os.sep) + os.sep self.__popen = popen self.__makedirs = makedirs diff --git a/pre_commit/run.py b/pre_commit/run.py index 706d68dd..a297ecc9 100644 --- a/pre_commit/run.py +++ b/pre_commit/run.py @@ -35,9 +35,13 @@ def run(argv): '--no-stash', default=False, action='store_true', help='Use this option to prevent auto stashing of unstaged files.', ) - run_parser.add_argument('--verbose', '-v', action='store_true', default=False) + run_parser.add_argument( + '--verbose', '-v', action='store_true', default=False, + ) - help = subparsers.add_parser('help', help='Show help for a specific command.') + help = subparsers.add_parser( + 'help', help='Show help for a specific command.' + ) help.add_argument('help_cmd', nargs='?', help='Command to show help for.') # Argparse doesn't really provide a way to use a `default` subparser diff --git a/setup.py b/setup.py index 3475922c..0bb0362b 100644 --- a/setup.py +++ b/setup.py @@ -4,7 +4,10 @@ from setuptools import setup setup( name='pre_commit', - description='A framework for managing and maintaining multi-language pre-commit hooks.', + description=( + 'A framework for managing and maintaining multi-language pre-commit ' + 'hooks.' + ), url='https://github.com/pre-commit/pre-commit', version='0.0.0', diff --git a/tests/clientlib/validate_base_test.py b/tests/clientlib/validate_base_test.py index 67cda358..5e85b0ee 100644 --- a/tests/clientlib/validate_base_test.py +++ b/tests/clientlib/validate_base_test.py @@ -44,7 +44,9 @@ def test_raises_for_invalid_yaml_file(noop_validator): def test_raises_for_failing_schema(array_validator): with pytest.raises(ValueError): - array_validator(get_resource_path('valid_yaml_but_invalid_manifest.yaml')) + array_validator( + get_resource_path('valid_yaml_but_invalid_manifest.yaml') + ) def test_passes_array_schema(array_validator): diff --git a/tests/commands_test.py b/tests/commands_test.py index af1f1b08..3d8f32e2 100644 --- a/tests/commands_test.py +++ b/tests/commands_test.py @@ -31,7 +31,9 @@ def test_install_pre_commit(empty_git_dir): assert ret == 0 assert os.path.exists(runner.pre_commit_path) pre_commit_contents = open(runner.pre_commit_path).read() - pre_commit_sh = pkg_resources.resource_filename('pre_commit', 'resources/pre-commit.sh') + pre_commit_sh = pkg_resources.resource_filename( + 'pre_commit', 'resources/pre-commit.sh', + ) expected_contents = open(pre_commit_sh).read() assert pre_commit_contents == expected_contents stat_result = os.stat(runner.pre_commit_path) @@ -132,7 +134,9 @@ def test_removes_defaults(out_of_date_repo, runner_with_mocked_store): assert 'expected_return_value' not in ret['hooks'][0] -def test_autoupdate_out_of_date_repo(out_of_date_repo, mock_out_store_directory): +def test_autoupdate_out_of_date_repo( + out_of_date_repo, mock_out_store_directory +): before = open(C.CONFIG_FILE).read() runner = Runner(out_of_date_repo.python_hooks_repo) ret = commands.autoupdate(runner) @@ -152,7 +156,10 @@ def hook_disappearing_repo(python_hooks_repo): config_wrapped = apply_defaults([config], CONFIG_JSON_SCHEMA) validate_config_extra(config_wrapped) config = config_wrapped[0] - shutil.copy(get_resource_path('manifest_without_foo.yaml'), C.MANIFEST_FILE) + shutil.copy( + get_resource_path('manifest_without_foo.yaml'), + C.MANIFEST_FILE, + ) local['git']['add', '.']() local['git']['commit', '-m', 'Remove foo']() @@ -167,14 +174,18 @@ def hook_disappearing_repo(python_hooks_repo): ) -def test_hook_disppearing_repo_raises(hook_disappearing_repo, runner_with_mocked_store): +def test_hook_disppearing_repo_raises( + hook_disappearing_repo, runner_with_mocked_store +): with pytest.raises(commands.RepositoryCannotBeUpdatedError): commands._update_repository( hook_disappearing_repo.repo_config, runner_with_mocked_store, ) -def test_autoupdate_hook_disappearing_repo(hook_disappearing_repo, mock_out_store_directory): +def test_autoupdate_hook_disappearing_repo( + hook_disappearing_repo, mock_out_store_directory +): before = open(C.CONFIG_FILE).read() runner = Runner(hook_disappearing_repo.python_hooks_repo) ret = commands.autoupdate(runner) @@ -206,7 +217,13 @@ def get_write_mock_output(write_mock): return ''.join(call[0][0] for call in write_mock.call_args_list) -def _get_opts(all_files=False, color=False, verbose=False, hook=None, no_stash=False): +def _get_opts( + all_files=False, + color=False, + verbose=False, + hook=None, + no_stash=False, +): return auto_namedtuple( all_files=all_files, color=color, @@ -234,7 +251,9 @@ def _test_run(repo, options, expected_outputs, expected_ret, stage): assert expected_output_part in printed -def test_run_all_hooks_failing(repo_with_failing_hook, mock_out_store_directory): +def test_run_all_hooks_failing( + repo_with_failing_hook, mock_out_store_directory +): _test_run( repo_with_failing_hook, {}, @@ -262,7 +281,14 @@ def test_run_all_hooks_failing(repo_with_failing_hook, mock_out_store_directory) ({}, ('Bash hook', '(no files to check)', 'Skipped'), 0, False), ) ) -def test_run(repo_with_passing_hook, options, outputs, expected_ret, stage, mock_out_store_directory): +def test_run( + repo_with_passing_hook, + options, + outputs, + expected_ret, + stage, + mock_out_store_directory, +): _test_run(repo_with_passing_hook, options, outputs, expected_ret, stage) @@ -275,7 +301,13 @@ def test_run(repo_with_passing_hook, options, outputs, expected_ret, stage, mock (False, False, True), ), ) -def test_no_stash(repo_with_passing_hook, no_stash, all_files, expect_stash, mock_out_store_directory): +def test_no_stash( + repo_with_passing_hook, + no_stash, + all_files, + expect_stash, + mock_out_store_directory, +): stage_a_file() # Make unstaged changes with open('foo.py', 'w') as foo_file: @@ -347,11 +379,15 @@ def test_skip_hook(repo_with_passing_hook, mock_out_store_directory): assert msg in printed -def test_hook_id_not_in_non_verbose_output(repo_with_passing_hook, mock_out_store_directory): +def test_hook_id_not_in_non_verbose_output( + repo_with_passing_hook, mock_out_store_directory +): ret, printed = _do_run(repo_with_passing_hook, _get_opts(verbose=False)) assert '[bash_hook]' not in printed -def test_hook_id_in_verbose_output(repo_with_passing_hook, mock_out_store_directory): +def test_hook_id_in_verbose_output( + repo_with_passing_hook, mock_out_store_directory +): ret, printed = _do_run(repo_with_passing_hook, _get_opts(verbose=True)) assert '[bash_hook] Bash hook' in printed diff --git a/tests/prefixed_command_runner_test.py b/tests/prefixed_command_runner_test.py index d7ee2ab5..2e4a586f 100644 --- a/tests/prefixed_command_runner_test.py +++ b/tests/prefixed_command_runner_test.py @@ -56,7 +56,9 @@ def test_init_normalizes_path_endings(input, expected_prefix): def test_run_substitutes_prefix(popen_mock, makedirs_mock): - instance = PrefixedCommandRunner('prefix', popen=popen_mock, makedirs=makedirs_mock) + instance = PrefixedCommandRunner( + 'prefix', popen=popen_mock, makedirs=makedirs_mock, + ) ret = instance.run(['{prefix}bar', 'baz'], retcode=None) popen_mock.assert_called_once_with( ['prefix/bar', 'baz'], @@ -104,7 +106,9 @@ def test_from_command_runner(prefix, path_end, expected_output): def test_from_command_runner_preserves_popen(popen_mock, makedirs_mock): - first = PrefixedCommandRunner('foo', popen=popen_mock, makedirs=makedirs_mock) + first = PrefixedCommandRunner( + 'foo', popen=popen_mock, makedirs=makedirs_mock, + ) second = PrefixedCommandRunner.from_command_runner(first, 'bar') second.run(['foo/bar/baz'], retcode=None) popen_mock.assert_called_once_with( diff --git a/tox.ini b/tox.ini index ca14ff82..87082d9e 100644 --- a/tox.ini +++ b/tox.ini @@ -23,6 +23,3 @@ deps = sphinx changedir = docs commands = sphinx-build -b html -d build/doctrees source build/html - -[flake8] -max-line-length=131 From 9b030681893fc24a5eec13cc8b3734810fc5e9f3 Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Fri, 6 Jun 2014 07:52:02 -0700 Subject: [PATCH 5/5] Remove most of five --- pre_commit/five.py | 68 --------------------------- pre_commit/prefixed_command_runner.py | 7 +-- 2 files changed, 2 insertions(+), 73 deletions(-) diff --git a/pre_commit/five.py b/pre_commit/five.py index e3bab85e..28db863f 100644 --- a/pre_commit/five.py +++ b/pre_commit/five.py @@ -8,71 +8,3 @@ if PY2: text = unicode # flake8: noqa else: text = str - - -def n(obj): - """Produce a native string. - - Similar in behavior to str(), but uses US-ASCII encoding when necessary. - """ - if isinstance(obj, str): - return obj - elif PY2 and isinstance(obj, text): - return obj.encode('US-ASCII') - elif PY3 and isinstance(obj, bytes): - return obj.decode('US-ASCII') - else: - return str(obj) - - -def u(obj): - """Produces text. - - Similar in behavior to str() in python3 or unicode() in python2, - but uses US-ASCII encoding when necessary. - """ - if isinstance(obj, text): - return obj - elif isinstance(obj, bytes): - return obj.decode('US-ASCII') - else: - return text(obj) - - -def b(obj): - """Produces bytes. - - Similar in behavior to bytes(), but uses US-ASCII encoding when necessary. - """ - if isinstance(obj, bytes): - return obj - elif isinstance(obj, text): - return obj.encode('US-ASCII') - else: - return bytes(obj) - - -def udict(*args, **kwargs): - """Similar to dict(), but keyword-keys are text.""" - kwargs = dict([ - (u(key), val) - for key, val in kwargs.items() - ]) - - return dict(*args, **kwargs) - -def ndict(*args, **kwargs): - """Similar to dict(), but keyword-keys are forced to native strings.""" - # I hate this :( - kwargs = dict([ - (n(key), val) - for key, val in kwargs.items() - ]) - - return dict(*args, **kwargs) - -def open(*args, **kwargs): - """Override the builtin open() to return text and use utf8 by default.""" - from io import open - kwargs.setdefault('encoding', 'UTF-8') - return open(*args, **kwargs) diff --git a/pre_commit/prefixed_command_runner.py b/pre_commit/prefixed_command_runner.py index 90678ddb..45acb7f1 100644 --- a/pre_commit/prefixed_command_runner.py +++ b/pre_commit/prefixed_command_runner.py @@ -2,8 +2,6 @@ import os import os.path import subprocess -from pre_commit import five - class CalledProcessError(RuntimeError): def __init__(self, returncode, cmd, expected_returncode, output=None): @@ -70,11 +68,10 @@ class PrefixedCommandRunner(object): replaced_cmd = _replace_cmd(cmd, prefix=self.prefix_dir) proc = self.__popen(replaced_cmd, **popen_kwargs) stdout, stderr = proc.communicate(stdin) - # TODO: stdout, stderr = from_bytes(stdout), from_bytes(stderr) if isinstance(stdout, bytes): - stdout = five.text(stdout, 'utf-8') + stdout = stdout.decode('UTF-8') if isinstance(stderr, bytes): - stderr = five.text(stderr, 'utf-8') + stderr = stderr.decode('UTF-8') returncode = proc.returncode if retcode is not None and retcode != returncode: