New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes file `types` from .pre-commit-config.yaml issue #744

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@zph

zph commented May 3, 2018

馃憢 Thanks for making pre-commit available :). It's quite handy!

This PR fixes a bug where files are not being recognized based on their types using configuration file setting types.

Technical writeup of findings: Commit message for 29d66f4

Issue / Reproduction case

When using a pre-commit-config.yaml in a repository with many bash scripts (that lack a .sh suffix to filename):

- repo: local
 - id: shellcheck
    name: 'Shellcheck Linter'
    entry: shellcheck
    language: system
    types:
      - shell
      - sh
      - bash

And running:

pre-commit run --all-files

Actual output:

Shellcheck Linter....................................(no files to check)Skipped

Expected behavior

That pre-commit finds the files with a shebang of #!/usr/bin/env bash and lints them.

Solution

I added two test cases, one for Python (which did behave properly) and one for a bash script like:

Filename: bash_script

#!/usr/bin/env bash
echo "Hello"

Then I tracked down the issue to the comparison between a List and a FrozenSet here:

if tags >= types and not tags & exclude_types:
  ret.append(filename)

I rewrote that comparison to compare two FrozenSets using the intersection function. This produces predictable results on Python 2.7 and 3.5.

Once this is approved, I'll happily squash it down to a single commit. Right now it's 2 commits to show the spec failing before the patch.

Notes:

  • I stubbed in a get_tags function via dependency injection to simplify testing. If you prefer a different mechanism, feel free to adjust the PR to suit your testing preferences.
  • I'm seeing unrelated test failures locally. I'll see how CI behaves and revise PR if they fail on CI.

zph added some commits May 3, 2018

Add tests for _filter_by_types
Adds a test for python and bash `types` using identify per interpreter
interpretation.

The python test succeeds with a false positive. The bash test identifies
the bug, where a comparison happens between `tags >= types`.

Tags is the tag list from identify, types is the value passed in from
configuration file as `acceptable types`.

The `tags(List) >= types(FrozenSet)` does a sort comparison of the two sequences
as evidenced by this code sample (which should return True):

```
>>> types = frozenset(['a'])
>>> tags = ['bash']
>>> tags >= types
True
```

Note: In 2.7, I cannot get this to return anything other than True.
Including the empty case:
```
>>> [] > frozenset()
True
```

Which means to me that a list is always >= a frozenset in Python 2.7.

In Python 3.5, I see errors when comparing a List to a FrozenSet. So
thankfully the interpreter is stricter in this case.

```
>>> [] >= frozenset()
TypeError: unorderable types: list() >= frozenset()
```
Fix `types` bug by using FrozenSet#intersection
Fixes a bug where files are not being recognized based on their `types`
using configuration file.

In the case of using `types: ['bash', 'sh', 'shell']` in
pre-commit-config, those filetypes will not be found, resulting in "no
files found" being reported with pre-commit. This persists even when
using --all-files flag.

Intersection produces predictable results in Python 2.7 and 3.x.
@zph

This comment has been minimized.

Show comment
Hide comment
@zph

zph May 3, 2018

@asottile Please let me know if you think this is my PR breaking the tests. I was unable to get them passing locally even before making changes.

zph commented May 3, 2018

@asottile Please let me know if you think this is my PR breaking the tests. I was unable to get them passing locally even before making changes.

@asottile

This comment has been minimized.

Show comment
Hide comment
@asottile

asottile May 3, 2018

Member

First off, thanks for the issue and PR!

Turns out the current behaviour is intentional (types are currently an AND relationship) -- this was useful for a few specific usecases when it was initially written (though I definitely went back and forth on it a few times when implementing #551!). Note that there's a current issue which matches roughly the description of your test here (as well as some potential implementation ideas there): #607

At least for your case, types: [shell] should be sufficient to match the same set of things you'd like to match.

Member

asottile commented May 3, 2018

First off, thanks for the issue and PR!

Turns out the current behaviour is intentional (types are currently an AND relationship) -- this was useful for a few specific usecases when it was initially written (though I definitely went back and forth on it a few times when implementing #551!). Note that there's a current issue which matches roughly the description of your test here (as well as some potential implementation ideas there): #607

At least for your case, types: [shell] should be sufficient to match the same set of things you'd like to match.

@pre-commit pre-commit deleted a comment from artcyb May 6, 2018

@zph

This comment has been minimized.

Show comment
Hide comment
@zph

zph May 15, 2018

@asottile Interesting. I misread the docs then about the AND behavior.

So if I want to do sh or bash etc as various possibilities, I need separate declarations for them? I tried out your suggestion and it works :). Merely surprising.

zph commented May 15, 2018

@asottile Interesting. I misread the docs then about the AND behavior.

So if I want to do sh or bash etc as various possibilities, I need separate declarations for them? I tried out your suggestion and it works :). Merely surprising.

@zph zph closed this May 15, 2018

@zph zph deleted the zph:bug/file-types-not-recognized branch May 15, 2018

@zph zph restored the zph:bug/file-types-not-recognized branch May 15, 2018

@asottile

This comment has been minimized.

Show comment
Hide comment
@asottile

asottile May 15, 2018

Member

@zph at least for sh and bash, there's a blanket shell which covers both of them.

I'd be happy if you chimed in on #607 and maybe hacked at an implementation -- definitely agree there's a need there :)

Member

asottile commented May 15, 2018

@zph at least for sh and bash, there's a blanket shell which covers both of them.

I'd be happy if you chimed in on #607 and maybe hacked at an implementation -- definitely agree there's a need there :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment