From 84c8dd5e8d192a2ed6c0555a2d00164934266ba3 Mon Sep 17 00:00:00 2001 From: Ian Cordasco Date: Fri, 6 Mar 2015 19:47:55 -0600 Subject: [PATCH] Simplify @mpenkov's fix for exclude in git hooks The simpler fix is to ensure that the filename isn't the full path to the temporary file before we check to see if it should be excluded or if it should even match. This also fixes a bug I found while testing the pull request in which no files are checked. --- flake8/hooks.py | 23 ++++----- flake8/tests/test_git_hook.py | 97 ----------------------------------- flake8/tests/test_hooks.py | 37 +++++++++++++ 3 files changed, 46 insertions(+), 111 deletions(-) delete mode 100644 flake8/tests/test_git_hook.py create mode 100644 flake8/tests/test_hooks.py diff --git a/flake8/hooks.py b/flake8/hooks.py index 1a15f0b..6687238 100644 --- a/flake8/hooks.py +++ b/flake8/hooks.py @@ -6,7 +6,7 @@ import sys import stat from subprocess import Popen, PIPE import shutil -from tempfile import mkdtemp +import tempfile try: from configparser import ConfigParser except ImportError: # Python 2 @@ -16,11 +16,6 @@ from flake8.engine import get_parser, get_style_guide from flake8.main import DEFAULT_CONFIG -def fix_exclude(flake8_style, tmpdir): - flake8_style.options.exclude = [tmpdir + x if "/" in x else x - for x in flake8_style.options.exclude] - - def git_hook(complexity=-1, strict=False, ignore=None, lazy=False): """This is the function used by the git hook. @@ -53,11 +48,10 @@ def git_hook(complexity=-1, strict=False, ignore=None, lazy=False): if complexity > -1: options['max_complexity'] = complexity - tmpdir = mkdtemp() + tmpdir = tempfile.mkdtemp() flake8_style = get_style_guide(config_file=DEFAULT_CONFIG, paths=['.'], **options) - fix_exclude(flake8_style, tmpdir) filepatterns = flake8_style.options.filename # Copy staged versions to temporary directory @@ -75,15 +69,16 @@ def git_hook(complexity=-1, strict=False, ignore=None, lazy=False): dirname = os.path.join(tmpdir, dirname) if not os.path.isdir(dirname): os.makedirs(dirname) - filename = os.path.join(dirname, filename) - # write staged version of file to temporary directory - with open(filename, "wb") as fh: - fh.write(out) # check_files() only does this check if passed a dir; so we do it - if ((pep8.filename_match(filename, filepatterns) and - not flake8_style.excluded(filename))): + if ((pep8.filename_match(file_, filepatterns) and + not flake8_style.excluded(file_))): + + filename = os.path.join(dirname, filename) files_to_check.append(filename) + # write staged version of file to temporary directory + with open(filename, "wb") as fh: + fh.write(out) # Run the checks report = flake8_style.check_files(files_to_check) diff --git a/flake8/tests/test_git_hook.py b/flake8/tests/test_git_hook.py deleted file mode 100644 index 3efa477..0000000 --- a/flake8/tests/test_git_hook.py +++ /dev/null @@ -1,97 +0,0 @@ -import unittest -import mock - -import flake8.hooks -import flake8.engine - - -def side_effect(command, raw_output=False, decode=False): - if command.startswith("git diff-index") and not raw_output: - return 0, ["ignore.py", "myproject/main.py", - "myproject/test/test.py"], [] - elif command.startswith("git show") and raw_output: - return 0, "dummy standard output", "dummy standard error" - else: - raise NotImplementedError("side_effect") - - -def mock_open(read_data=""): - # - # mock_open from older versions of mock do not have readline - # - # There is a patch for this issue (http://bugs.python.org/issue17467). - # I didn't use it directly since I needed readline to return None after all - # lines have been read. The multiple None objects appended at the end - # achieve this. I'm not sure why I need several -- one or two weren't - # enough. - # - mo = mock.mock_open(read_data=read_data) - lines = [x + "\n" for x in read_data.split("\n")] + [None]*10 - mo.return_value.readline.side_effect = lines - return mo - - -class TestFixExclude(unittest.TestCase): - """The scenario used to test fix_exclude is as follows. There is a git - repo with two files: ignore.py, myproject/main.py and - myproject/test/test.py. We want to run flake over myproject/main.py only. - To achieve this, we add myproject/test/test.py to the list of exclude - patterns -- in real life, this is done by editing configuration files - (http://flake8.readthedocs.org/en/latest/config.html), but for the test we - just mock it. - - Without the fix, since the exclude pattern for test.py contains a slash, - pep8.read_config will convert it to an absolute path, using os.curdir as a - base. This behavior causes pep8.filename_match to fail, since our files - will be in tmpdir, not os.curdir. The end result is flake8 ends up - checking myproject/test/test.py, which is something we don't want. - - With the fix, the exclude pattern is set correctly, and flake8 only checks - one file: myproject/main.py. - - The exclude pattern for ignore.py is not affected by the fix, since it - doesn't contain a slash, and isn't converted to an absolute path by - pep8.read_config. - """ - - def setUp(self): - self.config = "[flake8]\nexclude = ignore.py,myproject/test/test.py" - - @mock.patch("os.makedirs") - @mock.patch.object(flake8.engine.StyleGuide, "check_files") - @mock.patch("flake8.hooks.fix_exclude") - @mock.patch("flake8.hooks.mkdtemp", return_value="/tmp") - @mock.patch("flake8.hooks.run", side_effect=side_effect) - def test_without_fix(self, mock_run, mock_mkdtemp, mock_fix_exclude, - mock_check_files, mock_makedirs): - # - # http://www.voidspace.org.uk/python/mock/helpers.html#mock.mock_open - # - with mock.patch("ConfigParser.open", mock_open(read_data=self.config), - create=True): - with mock.patch("flake8.hooks.open", mock.mock_open(), create=True): - flake8.hooks.git_hook() - - self.assertEquals(mock_check_files.call_count, 1) - args = mock_check_files.call_args[0][0] - print args - self.assertEquals(len(args), 2) - - @mock.patch("os.makedirs") - @mock.patch.object(flake8.engine.StyleGuide, "check_files") - @mock.patch("flake8.hooks.mkdtemp", return_value="/tmp") - @mock.patch("flake8.hooks.run", side_effect=side_effect) - def test_with_fix(self, mock_run, mock_mkdtemp, mock_check_files, - mock_makedirs): - # - # http://www.voidspace.org.uk/python/mock/helpers.html#mock.mock_open - # - with mock.patch("ConfigParser.open", mock_open(read_data=self.config), - create=True): - with mock.patch("flake8.hooks.open", mock.mock_open(), create=True): - flake8.hooks.git_hook() - - self.assertEquals(mock_check_files.call_count, 1) - args = mock_check_files.call_args[0][0] - print args - self.assertEquals(len(args), 1) diff --git a/flake8/tests/test_hooks.py b/flake8/tests/test_hooks.py new file mode 100644 index 0000000..31fcd81 --- /dev/null +++ b/flake8/tests/test_hooks.py @@ -0,0 +1,37 @@ +"""Module containing the tests for flake8.hooks.""" +import os +import unittest + +try: + from unittest import mock +except ImportError: + import mock + +import flake8.hooks + + +def excluded(filename): + return filename.endswith('afile.py') + + +class TestGitHook(unittest.TestCase): + @mock.patch('os.makedirs') + @mock.patch('flake8.hooks.open', create=True) + @mock.patch('shutil.rmtree') + @mock.patch('tempfile.mkdtemp', return_value='/fake/tmp') + @mock.patch('flake8.hooks.run', + return_value=(None, ['foo/afile.py', 'foo/bfile.py'], None)) + @mock.patch('flake8.hooks.get_style_guide') + def test_prepends_tmp_directory_to_exclude(self, get_style_guide, run, + *args): + style_guide = get_style_guide.return_value = mock.Mock() + style_guide.options.exclude = ['foo/afile.py'] + style_guide.options.filename = ['foo/*'] + style_guide.excluded = excluded + + flake8.hooks.git_hook() + + dirname, filename = os.path.split(os.path.abspath('foo/bfile.py')) + tmpdir = os.path.join('/fake/tmp', dirname[1:]) + tmpfile = os.path.join(tmpdir, 'bfile.py') + style_guide.check_files.assert_called_once_with([tmpfile])