pre-commit / pre-commit-hooks Goto Github PK

Currently pep8 version 1.6.2 is installed into the virtual env's created for each repo. However, many of the plugins give a "DistributionNotFound: pep8<1.6,>=1.5.7" error and refuse to run.

This is after a "pre-commit clean" and "precommit autoupdate", and the SHA for the repo is 15b678e (the latest commit)

The trailing-whitespace hook strips all trailing whitespace, and will match files with a .md extension commonly used for Markdown.

While GitHub flavored Markdown (and other common variants) will ignore trailing whitespace, the original Markdown specification at Daring Fireball says:

When you do want to insert a <br /> break tag using Markdown, you end a line with two or more spaces, then type return.

emphasis mine

You could argue that this is a design/spec failure on the level of Make requiring TAB characters and not spaces for rule commands, but the reality is that there are Markdown files that use this feature, and at the very least there ought to be a flag for the trailing-whitespace-fixer that controls whether you want fix Markdown files (in addition to .md it is common to see .markdown extensions) by normalizing multiple spaces at the end of a non-blank line to 2 spaces or to remove them entirely (a single space at the end of a non-blank line, or any number of spaces on a line that contains only spaces, should always be removed).

I would suggest (and might even implement as a PR) something like --markdown-linebreak-ext=md,markdown as the default, indicating that files with .md or .markdown extensions would have multiple spaces at the end of a line normalized to two trailing spaces. If you wanted the trailing spaces stripped in all cases, you would use --markdown-linebreak-ext or --markdown-linebreak-ext= (or maybe --no-markdown-linebreak-ext), if you wanted special Markdown handling for other extensions or all files you would use --markdown-linebreak-ext=md,txt or --markdown-linebreak-ext=*.

This is a feature request.

Hooks that modify the contents can be dangerous and surprising. While some developers will appreciate the auto-fix behavior, others (like me) find it distracting and disconcerting. I'd prefer that none of the hooks perform any mutation and allow the user to take manual action. Allowing each hook to arbitrarily modify files burdens the user (me) with the need to understand what kinds of changes they might make, whether they are configurable or not, etc. It would be more ergonomic to opt-in to each mutation behavior.

Thanks for writing this framework. It is saving me lots of time!

As README.md says

Ignore PEP 8 violation types with args: ['-i', '--ignore=E000,...'], see .pre-commit-config.yaml in this repository for an example.

But I don't see this example in https://github.com/pre-commit/pre-commit-hooks/blob/master/.pre-commit-config.yaml.

My other hooks work, but this one always give:

$ pre-commit clean
Cleaned /Users/name/.pre-commit.
$ pre-commit install
pre-commit installed at /Users/name/Projects/name/.git/hooks/pre-commit
$ pre-commit run --all-files
An unexpected error has occurred: KeyError: 'requirements-txt-fixer'

My .pre-commit-config.yaml:

- repo: [email protected]:pre-commit/pre-commit-hooks.git
  sha: v0.3.0
  hooks:
  - id: check-json
  - id: check-yaml
  - id: flake8
  - id: requirements-txt-fixer
  - id: trailing-whitespace

Not sure if right place for this, but here goes.

It is unfortunately easy to accidentally check in secrets.

Has someone in the community already written a hook to watch out for private keys?

Either ignore that conflict pattern in .rst files or are a option to ignore certain file patterns (I don't want to list all the other file options).

We have a Puppet manifest with no aws key and the check keeps telling us that the file has aws credentials.

With the following config

    -   id: flake8
        args: ['--config=setup.cfg']

Any settings in setup.cfg are never read. Adding additional settings e.g. args: ['--config=setup.cfg', '--max-line-length=500'] will set max line length to 500 but will ignore anything set in setup.cfg.

Just added some images and a resources.qrc file to my PyQt4 project and got this error out of it. Is it because it tries to decode images as text?

Detect Private Key.......................................................Failed
hookid: detect-private-key

Traceback (most recent call last):

  File "C:\cygwin64\home\builder\.pre-commit\repo7jrxo4\py_env-default\Scripts\detect-private-key-script.py", line 9, in \<module\>

    load_entry_point('pre-commit-hooks==0.4.2', 'console_scripts', 'detect-private-key')()

  File "C:\cygwin64\home\builder\.pre-commit\repo7jrxo4\py_env-default\lib\site-packages\pre_commit_hooks\detect_private_key.py", line 17, in detect_private_key

    content = f.read()

  File "C:\cygwin64\home\builder\.pre-commit\repo7jrxo4\py_env-default\lib\encodings\cp1252.py", line 23, in decode

    return codecs.charmap_decode(input,self.errors,decoding_table)[0]

UnicodeDecodeError: 'charmap' codec can't decode byte 0x8f in position 44: character maps to \<undefined\>

Not sure if this sort of functionality makes sense to have in a hook, but I figured I'd suggest it and see what people think. I like to clean up my local repo now and then so that I don't have a ton of obsolete branches lying around. There are some scripts and tools people have written that do this, but you still have to remember to actually run them. Could be nice to just have it happen automatically when you're committing.

$ git status
On branch master

Initial commit

Changes to be committed:
  (use "git rm --cached <file>..." to unstage)

    new file:   TEST/foo
    new file:   test/bar

$ git commit -m "Test"
Check for case conflicts...................................................................................Passed
[master (root-commit) 6931ba5] Test
 2 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 TEST/foo
 create mode 100644 test/bar

$ cat .pre-commit-config.yaml
- repo: git://github.com/pre-commit/pre-commit-hooks
  sha: '0d88124ef6343fbbc6c9d2872853f73546c98a3f'
  hooks:
      - id: check-case-conflict

A conflict such as Test/foo and test/foo is caught though.

This can be a bit confusing (it'll still show the "Failed" text, but won't give an indication why)

cc @campaul

This might sound a little meta but it would be nice to have a hook that alphabetically sorts the hooks in the .pre-commit-config.yaml in my repo.

This is an unnecessary thing in python3

Should include some description of why this is potentially bad in the error message.

Something like:

'except: will also catch intentionally fatal exceptions such as KeyboardInterrupt and SystemExit. You probably want except Exception:, if you actually want to grab all exceptions including fatal ones, use except BaseException:'

Since the YAML loader is just checking syntax it shouldn't fail with custom types, e.g.:

---
some_value:
   !some_custom_type: "a string"

The pre-commit check fails even though this is legitimate YAML.

pre-commit run --all-files Passed,but git commit -m test failed.
autopep8 wrapper........................................................................................................................................Failed
hookid: autopep8-wrapper

Fixing collector/host_usage.py
And I found

    -   id: autopep8-wrapper
        args: ['-i', '--ignore=E265,E309,E501']

the args not working.

Hey, had just forked and was pulling my hair out wondering what I broke while I was testing a hook I wrote, turns out pyflakes just released 0.9.0, which causes me to get the below when trying to run any of my hooks when running fresh.

Traceback (most recent call last):
  File "/home/user/.pre-commit/repoHJDFOT/py_env-default/bin/trailing-whitespace-fixer", line 5, in <module>
    from pkg_resources import load_entry_point
  File "/home/user/.pre-commit/repoHJDFOT/py_env-default/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 3074, in <module>
    @_call_aside
  File "/home/user/.pre-commit/repoHJDFOT/py_env-default/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 3060, in _call_aside
    f(*args, **kwargs)
  File "/home/user/.pre-commit/repoHJDFOT/py_env-default/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 3087, in _initialize_master_working_set
    working_set = WorkingSet._build_master()
  File "/home/user/.pre-commit/repoHJDFOT/py_env-default/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 647, in _build_master
    return cls._build_from_requirements(__requires__)
  File "/home/user/.pre-commit/repoHJDFOT/py_env-default/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 660, in _build_from_requirements
    dists = ws.resolve(reqs, Environment())
  File "/home/user/.pre-commit/repoHJDFOT/py_env-default/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 838, in resolve
    raise VersionConflict(dist, req).with_context(dependent_req)
pkg_resources.ContextualVersionConflict: (pyflakes 0.9.0 (/home/user/.pre-commit/repoHJDFOT/py_env-default/lib/python2.7/site-packages), Requirement.parse('pyflakes<0.9,>=0.8.1'), set(['flake8']))

not sure who would need to tackle this so picking on the thing closest first. lemme know if i need to raise this with someone else.

Hi,

Currently this hook display an error but does not auto-fix Markdown files.
Hence a user will repeatedly get the following output

Trim Trailing Whitespace................................................................Failed
hookid: trailing-whitespace

Fixing README.md

The Fixing bit of the message is especially misleading: the hook actually does NOT try to remove extra trailing whitespaces in Markdown files, unless you pass it a --no-markdown-linebreak-ext argument.

Would you agree to improve the message here ?

For sake of example, consider a hook that checks C function signature against header files in some way.

Such a hook would run only on files matching \.c$ - but ideally would be triggered by those matching \.(c|h)$.

It would be awesome if in the hooks.yaml and .pre-commit-config.yaml files a hook could be configured similarly to:

files: \.c$
always_run: \.h$ 

I put it on always_run since I think that fits quite neatly - test_all_when or something might be a suitable alias:

• when true, the hook would run on all (not just staged) matching files, as it does currently;
• when a pattern, the hook would run on all (not just staged) matching files providing a staged file matches the always_run pattern

https://github.com/nvie/pip-tools includes a utility called pip-review that compares currently installed python packages to the latest versions available online (and optionally automatically updates them). I'm thinking it might be handy to have a hook that runs this (and updates requirements.txt?) automatically. Then again, pinned versions are often intentional due to compatibility issues with newer releases of packages. I'm opening this ticket to get opinions on whether something along these lines could possibly be a good idea.

In many workflows, you almost never want to commit to master. A lot of common workflows are based around committing to feature branches, then opening pull requests. For these workflows, it would be handy to have a hook that warns you if you accidentally try to commit to master and suggests switching to a branch (I know I've done this by accident before).

We've seen some bugs from contextmanagers that are written like:

@contextlib.contextmanager
def ctx(...):
    global g
    g = 1
    yield
    g = 0

Where they should have try: yield finally: to be exception-safe.

Without thinking too much about it I think we can use this heuristic:

• functions which are generators and are decorated by contextmanager or contextlib.contextmanager should either have a finally block or themselves contain a with statement

Since hhatto/autopep8#167 autopep8 has started reading the pep8 section from tox.ini or setup.cfg of a project. However, the autopep8 hook ignores this as it calls autopep8.parse_args() and autopep8.fix_code() without a second value (which defaults to False).

Any way we could get this to work?

A hook to detect and block on these would be nice:

<<<<<<< HEAD
=======
>>>>>>> master

https://docs.djangoproject.com/en/1.7/topics/testing/advanced/#using-different-testing-frameworks

Django's built-in test runner defaults to searching for test*.py. It would be nice if name-tests-test either allowed both the test*.py and *_test.py patterns or if there was a separate hook for the Django-style test naming pattern.

I'd be happy to submit a PR for the preferred solution/implementation.

Would really great for me. I appreciate the attention and the awesome tool. Managing hooks never will be easy as is with this tool.

pretty-format-json has an integer indent parameter that specifies the number of spaces to use (i.e., indentation width). This is an enhancement proposal, with the aim that tabs can be specified as indentation delimiter.

Since CPython 3.2 one can use strings instead of integers as indent argument. The main use for that is to specify tabs as indentation. This has some advantages and proponents (1, 2). For JSON, vertical alignment makes less sense than for programming language source code (e.g., long lists of parameter names), so a TAB-only indentation is feasible and advantageous, since it compresses the data (useful for webdev) and allows for configurable indentation width (useful for hard-to-read JSON).

This seems easy to implement by me or you, I'm just looking for feedback on this.

Running in Git Bash and Git Shell with pre-commit 0.4.1 both fail this way.

$ pre-commit run --all-files
Check for added large files..............................................←[41mFa
iled←[0m
hookid: check-added-large-files

xargs: check-added-large-files: Bad file number

Alternatively, files with a shebang should be executable

By default it uses the slow pure python implementation

We make this mistake all the time:

# foo.yaml

'key1': false
'key2': true
'key1': true

Currently end-of-file-fixer is limited to a certain set of file extensions. I have a .version file in one of my projects that was created by the pre-commit mirror maker script that doesn't have a newline at the end of the file but isn't fixed by end-of-file-fixer. Is there a reasonable way that end-of-file-fixer could run on any file that appears to be all plaintext without having to specify specific extensions to support?

Testing it on a simple file with a trailing space:

#' @export 

even when the file is staged,

pre-commit run --all-files -v

returns

Trim Trailing Whitespace.......................................................................................Passed

, but has no visible effect on the file.

I can't seem to get the autopep8-wrapper to work on my mac.

$ python --version
Python 2.7.6
$ pre-commit run --all-files
autopep8 wrapper....................................................................................................................................................................................Failed
hookid: autopep8-wrapper

usage: autopep8 [-h] [--version] [-v] [-d] [-i] [-r] [-j n] [-p n] [-a]
                [--experimental] [--exclude globs] [--list-fixes]
                [--ignore errors] [--select errors] [--max-line-length n]
                [--range line line] [--indent-size n]
                [files [files ...]]
autopep8: error: autopep8 only takes one filename as argument unless the "--in-place" or "--diff" args are used

https://github.com/google/yapf looks like a neat tool to potentially replace autopep8. It does appear to be a bit immature, but once it stabilizes I think it could make for a good hook.

Sometimes if I'm collaborating with someone on a branch in a remote repo, it's easy for branches to get out of sync when they push commits that conflict with my local commits. This can lead to messy conflict resolution. Could be nice to have a hook that checks to see if my local branch is up to date with the remote versions of the same branch, or even checks to see if my local branch could be in conflict with the remote master that it would eventually need to be merged into.

We've had some issues with bad files matches at yelp causing the hooks to not run at all. I think this is an easy way to surface that information

I've used Overcommit in the past. Unfortunately, it is currently unable to run hooks that alter files. Pre-commit has been working great for this. However, there is one feature I miss - the ability to reference only the altered lines in a hook.

For instance, you can compare the line number in an error message with the list of lines that are included in the diff, and then choose to ignore those unchanged lines. This can be very valuable in a legacy code base.

Are there any provisions for such a thing in Pre-commit?

In case sensitive filesystems, you could commit two files that have the same name but with different case (myfile.txt vs MyFile.txt), but then if these are checked out into a case insensitive filesystem, the results are unpredictable. A hook to warn or reject commits that result in these sorts of filenames would be nice.

when adding large files to git LFS, the check-large-files-added hook still complains. I think it would be best to add a flag to ignore LFS files, because this plugin is specifically used to solve the problem of adding large files to a git repo

For Django tests file names, tests_should_end_in_test.py will mistakenly think file names such as 'app/blah/tests/bad_django_test_fname.py' as valid django test names since the regex test_name_pattern = 'test.*.py' matches anything between between 'test' and '.py'- even '/' and other unintended characters.

Running in Git Bash and Git Shell with pre-commit 0.4.1 both fail this way.

$ pre-commit run --all-files
Check for case conflicts..............................................←[41mFa
iled←[0m
hookid: check-case-conflict

xargs: check-case-conflict: Bad file number

ekalinin/nodeenv#109

Currently both check_json and check_yaml only parse the file, but there are lots of easy mistakes you can make (especially with yaml) where the file is "valid yaml", but not at all what you want or expected.

If these checks took an optional param jsonschema which should be a file path to a jsonschema, they could validate the file against the jsonschema, without the need to write a custom hook for each format.

See: https://github.com/google/yapf

I've been using it in a custom Git precommit hook and so having it supported by pre-commit is sort of a requirement for me to transition to Yelp pre-commit for existing projects.