mirror of
https://github.com/PyCQA/flake8.git
synced 2026-03-30 10:46:54 +00:00
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.
This commit is contained in:
Ian Cordasco
Ian Cordasco
parent
de9fd7d6a2
commit
84c8dd5e8d
3 changed files with 46 additions and 111 deletions
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
37
flake8/tests/test_hooks.py
Normal file
37
flake8/tests/test_hooks.py
Normal file
|
|
@ -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])
|
||||
Loading…
Add table
Add a link
Reference in a new issue