From de33fad483a9c2b5a37db376b9fb965a9da76e5e Mon Sep 17 00:00:00 2001 From: Anthony Sottile Date: Wed, 18 Mar 2015 10:06:23 -0700 Subject: [PATCH] Autoupdate roundtrips. Resolves #210 --- pre_commit/clientlib/validate_base.py | 6 ++-- pre_commit/commands/autoupdate.py | 43 ++++++++------------------- pre_commit/constants.py | 7 ----- pre_commit/ordereddict.py | 7 ----- pre_commit/util.py | 9 ++++++ setup.py | 3 +- testing/fixtures.py | 31 +++++++++---------- tests/clientlib/validate_base_test.py | 10 ------- tests/commands/autoupdate_test.py | 33 ++++++++++++++------ tests/commands/run_test.py | 18 +++++------ tests/runner_test.py | 9 +++--- 11 files changed, 78 insertions(+), 98 deletions(-) delete mode 100644 pre_commit/ordereddict.py diff --git a/pre_commit/clientlib/validate_base.py b/pre_commit/clientlib/validate_base.py index 3d08a6c3..912f95d7 100644 --- a/pre_commit/clientlib/validate_base.py +++ b/pre_commit/clientlib/validate_base.py @@ -9,9 +9,9 @@ import sys import jsonschema import jsonschema.exceptions import pkg_resources -import yaml from pre_commit.jsonschema_extensions import apply_defaults +from pre_commit.util import yaml_load def is_regex_valid(regex): @@ -36,14 +36,14 @@ def get_validator( the object read from the file. The function should either raise exception_type on failure. """ - def validate(filename, load_strategy=yaml.load): + def validate(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 = load_strategy(file_contents) + obj = yaml_load(file_contents) except Exception as e: raise exception_type( 'Invalid yaml: {0}\n{1}'.format(os.path.relpath(filename), e), diff --git a/pre_commit/commands/autoupdate.py b/pre_commit/commands/autoupdate.py index a446a172..5298daaf 100644 --- a/pre_commit/commands/autoupdate.py +++ b/pre_commit/commands/autoupdate.py @@ -3,18 +3,14 @@ from __future__ import unicode_literals import sys -from aspy.yaml import ordered_dump -from aspy.yaml import ordered_load - -import pre_commit.constants as C from pre_commit.clientlib.validate_config import CONFIG_JSON_SCHEMA from pre_commit.clientlib.validate_config import is_local_hooks from pre_commit.clientlib.validate_config import load_config from pre_commit.jsonschema_extensions import remove_defaults -from pre_commit.ordereddict import OrderedDict from pre_commit.repository import Repository from pre_commit.util import cmd_output from pre_commit.util import cwd +from pre_commit.util import yaml_dump class RepositoryCannotBeUpdatedError(RuntimeError): @@ -37,10 +33,10 @@ def _update_repository(repo_config, runner): # Don't bother trying to update if our sha is the same if head_sha == repo_config['sha']: - return repo_config + return head_sha - # Construct a new config with the head sha - new_config = OrderedDict(repo_config) + # Modify the sha to be the head sha + new_config = dict(repo_config) new_config['sha'] = head_sha new_repo = Repository.create(new_config, runner.store) @@ -53,53 +49,40 @@ def _update_repository(repo_config, runner): '{0}'.format(', '.join(sorted(hooks_missing))) ) - return new_config + return head_sha def autoupdate(runner): """Auto-update the pre-commit config to the latest versions of repos.""" retv = 0 - output_configs = [] changed = False - input_configs = load_config( - runner.config_file_path, - load_strategy=ordered_load, - ) + configs = load_config(runner.config_file_path) - for repo_config in input_configs: + for repo_config in configs: if is_local_hooks(repo_config): - output_configs.append(repo_config) continue sys.stdout.write('Updating {0}...'.format(repo_config['repo'])) sys.stdout.flush() + original_sha = repo_config['sha'] try: - new_repo_config = _update_repository(repo_config, runner) + new_sha = _update_repository(repo_config, runner) except RepositoryCannotBeUpdatedError as error: print(error.args[0]) - output_configs.append(repo_config) retv = 1 continue - if new_repo_config['sha'] != repo_config['sha']: + if new_sha != original_sha: changed = True - print( - 'updating {0} -> {1}.'.format( - repo_config['sha'], new_repo_config['sha'], - ) - ) - output_configs.append(new_repo_config) + print('updating {0} -> {1}.'.format(original_sha, new_sha)) + repo_config['sha'] = new_sha else: print('already up to date.') - output_configs.append(repo_config) if changed: with open(runner.config_file_path, 'w') as config_file: config_file.write( - ordered_dump( - remove_defaults(output_configs, CONFIG_JSON_SCHEMA), - **C.YAML_DUMP_KWARGS - ) + yaml_dump(remove_defaults(configs, CONFIG_JSON_SCHEMA)) ) return retv diff --git a/pre_commit/constants.py b/pre_commit/constants.py index b89c29f8..645bb75d 100644 --- a/pre_commit/constants.py +++ b/pre_commit/constants.py @@ -4,10 +4,3 @@ from __future__ import unicode_literals CONFIG_FILE = '.pre-commit-config.yaml' MANIFEST_FILE = 'hooks.yaml' - -YAML_DUMP_KWARGS = { - 'default_flow_style': False, - # Use unicode - 'encoding': None, - 'indent': 4, -} diff --git a/pre_commit/ordereddict.py b/pre_commit/ordereddict.py deleted file mode 100644 index 2844cb46..00000000 --- a/pre_commit/ordereddict.py +++ /dev/null @@ -1,7 +0,0 @@ -from __future__ import absolute_import -from __future__ import unicode_literals - -try: - from collections import OrderedDict # noqa -except ImportError: # pragma: no cover (PY26) - from ordereddict import OrderedDict # noqa diff --git a/pre_commit/util.py b/pre_commit/util.py index 57303f56..bc5aa827 100644 --- a/pre_commit/util.py +++ b/pre_commit/util.py @@ -12,6 +12,7 @@ import tarfile import tempfile import pkg_resources +import ruamel.yaml from pre_commit import five from pre_commit import parse_shebang @@ -213,3 +214,11 @@ def rmtree(path): else: raise shutil.rmtree(path, ignore_errors=False, onerror=handle_remove_readonly) + + +yaml_load = functools.partial( + ruamel.yaml.load, Loader=ruamel.yaml.RoundTripLoader, +) +yaml_dump = functools.partial( + ruamel.yaml.dump, Dumper=ruamel.yaml.RoundTripDumper, encoding=None, +) diff --git a/setup.py b/setup.py index 72db3533..78cfd739 100644 --- a/setup.py +++ b/setup.py @@ -39,12 +39,11 @@ setup( ] }, install_requires=[ - 'aspy.yaml', 'cached-property', 'jsonschema', 'nodeenv>=0.11.1', 'pyterminalsize', - 'pyyaml', + 'ruamel.yaml', 'virtualenv', ], extras_require={ diff --git a/testing/fixtures.py b/testing/fixtures.py index 36c7400c..95d53fd0 100644 --- a/testing/fixtures.py +++ b/testing/fixtures.py @@ -5,17 +5,17 @@ import contextlib import io import os.path -from aspy.yaml import ordered_dump -from aspy.yaml import ordered_load +from ruamel.yaml.comments import CommentedMap import pre_commit.constants as C from pre_commit.clientlib.validate_config import CONFIG_JSON_SCHEMA from pre_commit.clientlib.validate_config import validate_config_extra from pre_commit.clientlib.validate_manifest import load_manifest from pre_commit.jsonschema_extensions import apply_defaults -from pre_commit.ordereddict import OrderedDict from pre_commit.util import cmd_output from pre_commit.util import cwd +from pre_commit.util import yaml_dump +from pre_commit.util import yaml_load from testing.util import copy_tree_to_path from testing.util import get_head_sha from testing.util import get_resource_path @@ -41,10 +41,10 @@ def make_repo(tempdir_factory, repo_source): def modify_manifest(path): """Modify the manifest yielded by this context to write to hooks.yaml.""" manifest_path = os.path.join(path, C.MANIFEST_FILE) - manifest = ordered_load(io.open(manifest_path).read()) + manifest = yaml_load(io.open(manifest_path).read()) yield manifest with io.open(manifest_path, 'w') as manifest_file: - manifest_file.write(ordered_dump(manifest, **C.YAML_DUMP_KWARGS)) + manifest_file.write(yaml_dump(manifest)) cmd_output('git', 'commit', '-am', 'update hooks.yaml', cwd=path) @@ -54,18 +54,18 @@ def modify_config(path='.', commit=True): .pre-commit-config.yaml """ config_path = os.path.join(path, C.CONFIG_FILE) - config = ordered_load(io.open(config_path).read()) + config = yaml_load(io.open(config_path).read()) yield config with io.open(config_path, 'w', encoding='UTF-8') as config_file: - config_file.write(ordered_dump(config, **C.YAML_DUMP_KWARGS)) + config_file.write(yaml_dump(config)) if commit: cmd_output('git', 'commit', '-am', 'update config', cwd=path) def config_with_local_hooks(): - return OrderedDict(( + return CommentedMap(( ('repo', 'local'), - ('hooks', [OrderedDict(( + ('hooks', [CommentedMap(( ('id', 'do_not_commit'), ('name', 'Block if "DO NOT COMMIT" is found'), ('entry', 'DO NOT COMMIT'), @@ -77,13 +77,10 @@ def config_with_local_hooks(): def make_config_from_repo(repo_path, sha=None, hooks=None, check=True): manifest = load_manifest(os.path.join(repo_path, C.MANIFEST_FILE)) - config = OrderedDict(( - ('repo', repo_path), + config = CommentedMap(( ('sha', sha or get_head_sha(repo_path)), - ( - 'hooks', - hooks or [OrderedDict((('id', hook['id']),)) for hook in manifest], - ), + ('repo', repo_path), + ('hooks', hooks or [{'id': hook['id']} for hook in manifest]), )) if check: @@ -96,10 +93,10 @@ def make_config_from_repo(repo_path, sha=None, hooks=None, check=True): def write_config(directory, config): if type(config) is not list: - assert type(config) is OrderedDict + assert type(config) is CommentedMap config = [config] with io.open(os.path.join(directory, C.CONFIG_FILE), 'w') as config_file: - config_file.write(ordered_dump(config, **C.YAML_DUMP_KWARGS)) + config_file.write(yaml_dump(config)) def add_config_to_repo(git_path, config): diff --git a/tests/clientlib/validate_base_test.py b/tests/clientlib/validate_base_test.py index 5c44ab51..7e1fc9da 100644 --- a/tests/clientlib/validate_base_test.py +++ b/tests/clientlib/validate_base_test.py @@ -1,10 +1,8 @@ from __future__ import unicode_literals import pytest -from aspy.yaml import ordered_load from pre_commit.clientlib.validate_base import get_validator -from pre_commit.ordereddict import OrderedDict from testing.util import get_resource_path @@ -63,11 +61,3 @@ def test_raises_when_additional_validation_fails(additional_validator): def test_returns_object_after_validating(noop_validator): ret = noop_validator(get_resource_path('array_yaml_file.yaml')) assert ret == ['foo', 'bar'] - - -def test_load_strategy(noop_validator): - ret = noop_validator( - get_resource_path('ordering_data_test.yaml'), - load_strategy=ordered_load, - ) - assert type(ret) is OrderedDict diff --git a/tests/commands/autoupdate_test.py b/tests/commands/autoupdate_test.py index bd8fbe80..f17e1a2e 100644 --- a/tests/commands/autoupdate_test.py +++ b/tests/commands/autoupdate_test.py @@ -1,5 +1,6 @@ from __future__ import unicode_literals +import io import shutil import pytest @@ -9,7 +10,6 @@ from pre_commit.clientlib.validate_config import load_config from pre_commit.commands.autoupdate import _update_repository from pre_commit.commands.autoupdate import autoupdate from pre_commit.commands.autoupdate import RepositoryCannotBeUpdatedError -from pre_commit.ordereddict import OrderedDict from pre_commit.runner import Runner from pre_commit.util import cmd_output from pre_commit.util import cwd @@ -33,7 +33,7 @@ def test_up_to_date_repo(up_to_date_repo, runner_with_mocked_store): config = make_config_from_repo(up_to_date_repo) input_sha = config['sha'] ret = _update_repository(config, runner_with_mocked_store) - assert ret['sha'] == input_sha + assert ret == input_sha def test_autoupdate_up_to_date_repo( @@ -72,12 +72,12 @@ def test_out_of_date_repo(out_of_date_repo, runner_with_mocked_store): out_of_date_repo.path, sha=out_of_date_repo.original_sha, ) ret = _update_repository(config, runner_with_mocked_store) - assert ret['sha'] != out_of_date_repo.original_sha - assert ret['sha'] == out_of_date_repo.head_sha + assert ret != out_of_date_repo.original_sha + assert ret == out_of_date_repo.head_sha def test_autoupdate_out_of_date_repo( - out_of_date_repo, in_tmpdir, mock_out_store_directory + out_of_date_repo, in_tmpdir, mock_out_store_directory, ): # Write out the config config = make_config_from_repo( @@ -85,10 +85,10 @@ def test_autoupdate_out_of_date_repo( ) write_config('.', config) - before = open(C.CONFIG_FILE).read() + before = io.open(C.CONFIG_FILE).read() runner = Runner('.') ret = autoupdate(runner) - after = open(C.CONFIG_FILE).read() + after = io.open(C.CONFIG_FILE).read() assert ret == 0 assert before != after # Make sure we don't add defaults @@ -96,6 +96,21 @@ def test_autoupdate_out_of_date_repo( assert out_of_date_repo.head_sha in after +def test_autoupdate_preserves_comments( + out_of_date_repo, in_tmpdir, mock_out_store_directory, +): + config = make_config_from_repo( + out_of_date_repo.path, sha=out_of_date_repo.original_sha, check=False, + ) + write_config('.', config) + with io.open(C.CONFIG_FILE, 'a') as config_file: + config_file.write("# I'm a comment!\n") + runner = Runner('.') + autoupdate(runner) + after = io.open(C.CONFIG_FILE).read() + assert after.endswith("# I'm a comment!\n") + + @pytest.yield_fixture def hook_disappearing_repo(tempdir_factory): path = make_repo(tempdir_factory, 'python_hooks_repo') @@ -118,7 +133,7 @@ def test_hook_disppearing_repo_raises( config = make_config_from_repo( hook_disappearing_repo.path, sha=hook_disappearing_repo.original_sha, - hooks=[OrderedDict((('id', 'foo'),))], + hooks=[{'id': 'foo'}], ) with pytest.raises(RepositoryCannotBeUpdatedError): _update_repository(config, runner_with_mocked_store) @@ -130,7 +145,7 @@ def test_autoupdate_hook_disappearing_repo( config = make_config_from_repo( hook_disappearing_repo.path, sha=hook_disappearing_repo.original_sha, - hooks=[OrderedDict((('id', 'foo'),))], + hooks=[{'id': 'foo'}], check=False, ) write_config('.', config) diff --git a/tests/commands/run_test.py b/tests/commands/run_test.py index d23882e7..f68d57f5 100644 --- a/tests/commands/run_test.py +++ b/tests/commands/run_test.py @@ -10,6 +10,7 @@ import sys import mock import pytest +from ruamel.yaml.comments import CommentedMap import pre_commit.constants as C from pre_commit.commands.install_uninstall import install @@ -17,7 +18,6 @@ from pre_commit.commands.run import _get_skips from pre_commit.commands.run import _has_unmerged_paths from pre_commit.commands.run import get_changed_files from pre_commit.commands.run import run -from pre_commit.ordereddict import OrderedDict from pre_commit.output import sys_stdout_write_wrapper from pre_commit.runner import Runner from pre_commit.util import cmd_output @@ -454,16 +454,16 @@ def test_local_hook_for_stages( hook_stage, expected_output ): - config = OrderedDict(( + config = CommentedMap(( ('repo', 'local'), - ('hooks', (OrderedDict(( + ('hooks', (CommentedMap(( ('id', 'pylint'), ('name', 'hook 1'), ('entry', 'python -m pylint.__main__'), ('language', 'system'), ('files', r'\.py$'), ('stages', stage_for_first_hook) - )), OrderedDict(( + )), CommentedMap(( ('id', 'do_not_commit'), ('name', 'hook 2'), ('entry', 'DO NOT COMMIT'), @@ -488,15 +488,15 @@ def test_local_hook_for_stages( def test_local_hook_passes(repo_with_passing_hook, mock_out_store_directory): - config = OrderedDict(( + config = CommentedMap(( ('repo', 'local'), - ('hooks', (OrderedDict(( + ('hooks', (CommentedMap(( ('id', 'pylint'), ('name', 'PyLint'), ('entry', 'python -m pylint.__main__'), ('language', 'system'), ('files', r'\.py$'), - )), OrderedDict(( + )), CommentedMap(( ('id', 'do_not_commit'), ('name', 'Block if "DO NOT COMMIT" is found'), ('entry', 'DO NOT COMMIT'), @@ -520,9 +520,9 @@ def test_local_hook_passes(repo_with_passing_hook, mock_out_store_directory): def test_local_hook_fails(repo_with_passing_hook, mock_out_store_directory): - config = OrderedDict(( + config = CommentedMap(( ('repo', 'local'), - ('hooks', [OrderedDict(( + ('hooks', [CommentedMap(( ('id', 'no-todo'), ('name', 'No TODO'), ('entry', 'sh -c "! grep -iI todo $@" --'), diff --git a/tests/runner_test.py b/tests/runner_test.py index 782e8d53..39315ce8 100644 --- a/tests/runner_test.py +++ b/tests/runner_test.py @@ -4,8 +4,9 @@ from __future__ import unicode_literals import os import os.path +from ruamel.yaml.comments import CommentedMap + import pre_commit.constants as C -from pre_commit.ordereddict import OrderedDict from pre_commit.runner import Runner from pre_commit.util import cmd_output from pre_commit.util import cwd @@ -56,16 +57,16 @@ def test_repositories(tempdir_factory, mock_out_store_directory): def test_local_hooks(tempdir_factory, mock_out_store_directory): - config = OrderedDict(( + config = CommentedMap(( ('repo', 'local'), - ('hooks', (OrderedDict(( + ('hooks', (CommentedMap(( ('id', 'arg-per-line'), ('name', 'Args per line hook'), ('entry', 'bin/hook.sh'), ('language', 'script'), ('files', ''), ('args', ['hello', 'world']), - )), OrderedDict(( + )), CommentedMap(( ('id', 'do_not_commit'), ('name', 'Block if "DO NOT COMMIT" is found'), ('entry', 'DO NOT COMMIT'),