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

Types #551

Merged
merged 4 commits into from Jul 3, 2017

Conversation

Projects
None yet
3 participants
@asottile
Member

asottile commented Jul 2, 2017

This is the big one!

Should resolve the following issues:

Some notable things:

  • files and types are now both optional -- I'm not sure if this is ok? I'm considering making these inclusive-or-required (with always_run). files defaults to '' (matching all) and types defaults to ['file'] matching only files (this excludes submodules and symlinks)
  • files and types are evaluated with and, that is, for a file to be linted it must match both the files regex and the types listed.
  • Each tag inside types is evaluated with and -- that is, for a file to be chosen for linting it must be classified as all of the tags.
  • A separate library (identify) was chosen to allow the inevitable rapid updates to classification without encumbering the pre-commit release schedule.
  • The implementation allows for extension of "tags" by pre-commit itself, I think something cool that pre-commit could do is a newly-added tag (would cut out a lot of boilerplate in several hooks).

Known breaking changes:

  • pre-commit-hooks#check-symlinks - this hook intentionally lints symlink "files" and will now need types: ['symlink'] to work properly
  • always_run without files now defaults to files: '' instead of files: '^$' -- I think this actually makes more sense given the choices in the rest of the framework and is a "least surprise" outcome here (it also makes the code simple!)

@chriskuehl and I have put a ton of work into this (my branch is roughly based on his prototype branch) 馃帺 hattip

@Lucas-C is probably interested in this PR so... hi!

Show outdated Hide outdated pre_commit/commands/run.py
types = frozenset(types)
return tuple(
filename for filename in filenames
if tags_from_path(filename) >= types

This comment has been minimized.

@Lucas-C

Lucas-C Jul 2, 2017

Contributor

Nice ! 馃憤

@Lucas-C

Lucas-C Jul 2, 2017

Contributor

Nice ! 馃憤

Show outdated Hide outdated tests/commands/run_test.py
ret, printed = _do_run(cap_out, git_path, args)
assert ret == 1
assert b'bar.py' in printed
assert b'bar.notpy' not in printed

This comment has been minimized.

@Lucas-C

Lucas-C Jul 2, 2017

Contributor

The new types_repo/bin/hook.sh file is not tested ?

@Lucas-C

Lucas-C Jul 2, 2017

Contributor

The new types_repo/bin/hook.sh file is not tested ?

This comment has been minimized.

@asottile

asottile Jul 2, 2017

Member

All hook.sh does is print the filenames and invariantly fail -- I think this tests that?

@asottile

asottile Jul 2, 2017

Member

All hook.sh does is print the filenames and invariantly fail -- I think this tests that?

This comment has been minimized.

@Lucas-C

Lucas-C Jul 2, 2017

Contributor

Yes sure. Actually I thought hook.sh role was to test shebang detection.
My mistake

@Lucas-C

Lucas-C Jul 2, 2017

Contributor

Yes sure. Actually I thought hook.sh role was to test shebang detection.
My mistake

@Lucas-C

This comment has been minimized.

Show comment
Hide comment
@Lucas-C

Lucas-C Jul 2, 2017

Contributor

Great ! Thanks a lot for your efforts !

Contributor

Lucas-C commented Jul 2, 2017

Great ! Thanks a lot for your efforts !

@chriskuehl

This looks really great -- much simpler than my original WIP branch (granted, the hard part of that was wrangling jsonschema...).

A couple thoughts:

  • It might be useful to have pre-commit add a "submodule" tag at some point. Right now I suspect it's going to pass them to identify which will call them ['directory']. That's definitely an improvement since it won't break hooks like it does now, but probably could be better. identify isn't in a good place to determine what is a submodule since it doesn't know about git.
  • Probably worth mentioning identify-cli when updating the docs, as it should be pretty helpful to folks figuring out what tags they can use.
raise schema.ValidationError(
'Type tag {!r} is not recognized. '
'Try upgrading identify and pre-commit?'.format(tag),
)

This comment has been minimized.

@chriskuehl

chriskuehl Jul 2, 2017

Member

I have a feeling that we're going to have a fair number of people asking how to do this upgrade. Since often we don't pin pre-commit, the answer is usually going to be to rm the virtualenv and rebuild it... probably fine, just something to think about.

@chriskuehl

chriskuehl Jul 2, 2017

Member

I have a feeling that we're going to have a fair number of people asking how to do this upgrade. Since often we don't pin pre-commit, the answer is usually going to be to rm the virtualenv and rebuild it... probably fine, just something to think about.

This comment has been minimized.

@asottile

asottile Jul 2, 2017

Member

Yeah... the more common errors they're going to get is "no file hooks.yaml" and "missing key 'files'" though -- and unfortunately no way to retroactively improve error messages in old versions :S

@asottile

asottile Jul 2, 2017

Member

Yeah... the more common errors they're going to get is "no file hooks.yaml" and "missing key 'files'" though -- and unfortunately no way to retroactively improve error messages in old versions :S

Show outdated Hide outdated pre_commit/commands/run.py
types = frozenset(types)
return tuple(
filename for filename in filenames
if tags_from_path(filename) >= types

This comment has been minimized.

@chriskuehl

chriskuehl Jul 2, 2017

Member

Do you think it makes sense for the list of tags to be an OR rather than an AND?

I'm wondering if there's a case where someone will do something like types: [python, ruby] and be surprised that it matches nothing rather than all python and ruby files.

I'm having trouble coming up with a scenario where I'd want to target only files which have multiple tags... but I also can't really think of too many cases where I'd want to target both python and ruby but not just text, so maybe both cases are pretty rare?

My original (much more complicated) branch just checked for any intersection of the tags, but I didn't really spend much time considering the alternative.

@chriskuehl

chriskuehl Jul 2, 2017

Member

Do you think it makes sense for the list of tags to be an OR rather than an AND?

I'm wondering if there's a case where someone will do something like types: [python, ruby] and be surprised that it matches nothing rather than all python and ruby files.

I'm having trouble coming up with a scenario where I'd want to target only files which have multiple tags... but I also can't really think of too many cases where I'd want to target both python and ruby but not just text, so maybe both cases are pretty rare?

My original (much more complicated) branch just checked for any intersection of the tags, but I didn't really spend much time considering the alternative.

This comment has been minimized.

@asottile

asottile Jul 2, 2017

Member

Yeah I do think it should be AND - I also think I should implement exclude_types as we have (files, exclude) but no way to do that with types.

@asottile

asottile Jul 2, 2017

Member

Yeah I do think it should be AND - I also think I should implement exclude_types as we have (files, exclude) but no way to do that with types.

This comment has been minimized.

@asottile

asottile Jul 2, 2017

Member

Actually, maybe it should be OR - though I can't think of a compelling reason why someone would ever use more than one tag.

@asottile

asottile Jul 2, 2017

Member

Actually, maybe it should be OR - though I can't think of a compelling reason why someone would ever use more than one tag.

This comment has been minimized.

@chriskuehl

chriskuehl Jul 2, 2017

Member

Yeah, I can't really come up with a real use-case for either. It's still possible to get the OR behavior by specifying the hook twice if you really need it. I'm fine with either way.

fwiw all of the standard hooks were easy to write with just one type: https://github.com/chriskuehl/pre-commit-hooks/blob/new-typers/hooks.yaml

@chriskuehl

chriskuehl Jul 2, 2017

Member

Yeah, I can't really come up with a real use-case for either. It's still possible to get the OR behavior by specifying the hook twice if you really need it. I'm fine with either way.

fwiw all of the standard hooks were easy to write with just one type: https://github.com/chriskuehl/pre-commit-hooks/blob/new-typers/hooks.yaml

This comment has been minimized.

@asottile

asottile Jul 3, 2017

Member

shrugs, easier to leave it how it is so I'll do that. If someone has a legitimate usecase for OR that neither exclude_types nor separate hooks satisfies we can revisit this.

@asottile

asottile Jul 3, 2017

Member

shrugs, easier to leave it how it is so I'll do that. If someone has a legitimate usecase for OR that neither exclude_types nor separate hooks satisfies we can revisit this.

@asottile

This comment has been minimized.

Show comment
Hide comment
@asottile

asottile Jul 3, 2017

Member

Depending on how migrated repositories are, I'd suggest the following (at least temporary) if you plan to move repositories to use types:

if you still are maintaining hooks.yaml

Convert your hooks.yaml to a skeleton providing the minimum possible to satisfy <0.15.0 and add a minimum_pre_commit_version: 0.15.0 key to each hook.

For example (original here):

-   id: autopep8-wrapper
    language: system
    name: upgrade-your-pre-commit-version
    entry: upgrade-your-pre-commit-version
    files: ''
    minimum_pre_commit_version: 0.15.0
  • For pre-commit<0.6.7, an error about not being able to execute upgrade-your-pre-commit-version will be produced
  • For pre-commit>=0.6.7,<0.12.0 an error suggesting an upgrade
  • For pre-commit>=0.12.0, .pre-commit-hooks.yaml will be used instead (see below)

forward-backward compatible .pre-commit-hooks.yaml

  • Add minimum_pre_commit_version: 0.15.0 to each hook using types
  • Change files: ... to files: '' (this is the default value when files is missing)
  • The entire reason for the noop files is old versions of pre-commit won't even load the configuration far enough to produce a useful error message on minimum_pre_commit_version

For example (original here):

-   id: check-ast
    name: Check python ast
    description: Simply check whether the files parse as valid python.
    entry: check-ast
    language: python
    types: [python]
    # for backward compatibility
    files: ''
    minimum_pre_commit_version: 0.15.0
  • pre-commit<0.12.0 won't read this file
  • pre-commit>0.12.0,<=0.15.0 will suggest an upgrade
  • pre-commit>=0.15.0 will use types and files will be a noop

When you don't care about old versions, see below

once you no longer care about old versions of pre-commit

  • Remove the noop files: '' and the minimum_pre_commit_version.
Member

asottile commented Jul 3, 2017

Depending on how migrated repositories are, I'd suggest the following (at least temporary) if you plan to move repositories to use types:

if you still are maintaining hooks.yaml

Convert your hooks.yaml to a skeleton providing the minimum possible to satisfy <0.15.0 and add a minimum_pre_commit_version: 0.15.0 key to each hook.

For example (original here):

-   id: autopep8-wrapper
    language: system
    name: upgrade-your-pre-commit-version
    entry: upgrade-your-pre-commit-version
    files: ''
    minimum_pre_commit_version: 0.15.0
  • For pre-commit<0.6.7, an error about not being able to execute upgrade-your-pre-commit-version will be produced
  • For pre-commit>=0.6.7,<0.12.0 an error suggesting an upgrade
  • For pre-commit>=0.12.0, .pre-commit-hooks.yaml will be used instead (see below)

forward-backward compatible .pre-commit-hooks.yaml

  • Add minimum_pre_commit_version: 0.15.0 to each hook using types
  • Change files: ... to files: '' (this is the default value when files is missing)
  • The entire reason for the noop files is old versions of pre-commit won't even load the configuration far enough to produce a useful error message on minimum_pre_commit_version

For example (original here):

-   id: check-ast
    name: Check python ast
    description: Simply check whether the files parse as valid python.
    entry: check-ast
    language: python
    types: [python]
    # for backward compatibility
    files: ''
    minimum_pre_commit_version: 0.15.0
  • pre-commit<0.12.0 won't read this file
  • pre-commit>0.12.0,<=0.15.0 will suggest an upgrade
  • pre-commit>=0.15.0 will use types and files will be a noop

When you don't care about old versions, see below

once you no longer care about old versions of pre-commit

  • Remove the noop files: '' and the minimum_pre_commit_version.

@asottile asottile merged commit 3c88531 into master Jul 3, 2017

5 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

@asottile asottile deleted the types branch Jul 3, 2017

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