Integration as a http://pre-commit.com/ hook

[asottile]

This isn't really such a bug report as more of an inquisition about how to approach this.

As requested here: pre-commit/pre-commit-hooks#50 we've had some interest in integrating yapf with our pre-commit framework.

There's two basic approaches to doing this so I'd like to get your opinions on this (I'd like to contribute to making either of them happen):

1. Provide the proper metadata in google/yapf for running hooks (basically involves checking in a file similar to this: https://github.com/pre-commit/pre-commit-hooks/blob/master/hooks.yaml (except of course it'd just list yapf)).
2. Provide library mechanisms to fix files and we'd write a wrapper around yapf in one of our repositories.

Option 1 is ideal for us as we don't have to deal with dependencies, breaking apis, etc. especially when the project is in an early stage and potentially volatile (and I don't think yapf is on pypi yet?)

Either way, the general interface for a pre-commit hook to work with our framework is as follows:

• Installable in an isolated environment
• Exposes an executable which:
  • Takes a list of filenames to check / fix (in the case of yapf, either)
  • Returns 0 when everything is A-OK or no changes are made, and nonzero otherwise

I'm not sure how much of that already happens but I'd be willing to pitch in to make it work (and test it to prevent regressions, etc.).

Given those assumptions it'd be as simple as checking in a file named hooks.yaml which has contents probably similar to this:

-   id: yapf
    name: yapf
    description: "Runs yapf 'Yet Another Python Formatter' over python source"
    entry: yapf
    language: python
    files: \.py$
    args: [-i]

Thanks in advance,

Anthony

[bwendling]

Hi Anthony,

I agree that it's way too early to be depending upon our APIs to be stable. :-) My GitHub-fu is not all that great, so I'll let Eli comment on how the pre-commit hooks should work. Though option (1) sounds like the best way to do it.

One thing you should do before integrating YAPF into your project is go over the current PRs. There are a few PEP 8 violations that we haven't yet addressed. (Obligatory plug for contributions. :-) )

[eliben]

@asottile It's not clear to me what yapf-side changes are you proposing here, if any? Checking a file into googe/yapf just for the sake of your pre-commits projects doesn't sound right to me.

[jab]

@eliben It's not necessarily just for the sake of the pre-commits project. If you look at a hook.yml file, all it is is some metadata that specifies the name, description, file type operated on (e.g. *.py), and default args (e.g. -i) that yapf would be run with in a pre-commit context. Anyone wanting to use yapf as a pre-commit hook (which is a common use case for a tool like yapf), whether or not they actually use the pre-commit project to do it, could find it useful. I don't think this has been standardized yet, but it would make sense for any tool like yapf that wants to declare itself commit-hook friendly to include some metadata like this.

[eliben]

@jab As you say, this hasn't been standardized yet. I say let's wait until this is standartized and the same way to run yapf on precommit is used in a bunch of projects; at that point it will make sense to refactor this config into yapf itself.

[asottile]

@eliben Let me clarify. The configuration file format is standardized. The config file is just metadata in your repository that allows your project to be integrated. There's quite a few projects with this file checked in and we'd like to make yapf be one of them: http://pre-commit.com/hooks.html

Even if you don't want to do that, you didn't address my questions.

[eliben]

@asottile

Ah, sorry about that.

So yeah, seems like option (2) is the way to go. YAPF exposes a programmatic API that should be fairly stable -- see https://github.com/google/yapf/blob/master/yapf/yapflib/yapf_api.py

LMK if you have any questions about / issues with using this API

[asottile]

@eliben If I submit a tested PR which makes yapf return nonzero when changing files, and adds the hooks.yaml metadata we need would it be accepted? Using APIs is nice except when they break and I'm getting mixed signals between you and @gwelymernans about the stability (and frankly, we've been burned in the past with autopep8's api in a similar situation).

[eliben]

It's not clear what you mean by "makes yapf return nonzero when changing files", can you clarify?

Adding new files to the yapf repository for this purpose is not going to be accepted at this time. We may reconsider later.

[jab]

It's not clear what you mean by "makes yapf return nonzero when changing files", can you clarify?

Pretty sure this means the yapf process should exit 0 when there were no changes and exit nonzero when there were. That way a script invoking yapf can easily tell when it made changes. Make sense?

Adding new files to the yapf repository for this purpose is not going to be accepted at this time. We may reconsider later.

FWIW, I'm an early adopter of yapf and already contributed some bug reports and would contribute to the project more in the future. From my perspective, by adding a standard 5-line file with just some metadata about how yapf is called that makes it easier to integrate with other tools, yapf can only benefit by having more people try it out and contribute to it. So I hope you do reconsider! </$.02>

[bwendling]

On Wed, Apr 1, 2015 at 11:17 AM jab notifications@github.com wrote:

It's not clear what you mean by "makes yapf return nonzero when changing
files", can you clarify?

Pretty sure this means the yapf process should exit 0 when there were no
changes and exit nonzero when there were. That way a script invoking yapf
can easily tell when it made changes. Make sense?

This doesn't really make sense to me. An exit of non-0 traditionally means
that something went wrong (or in the case of "grep" it didn't find
anything). How is the script going to run YAPF? Will it use YAPF's APIs? or
will it execute it via a spawned process? If the former, then it should be
easy to modify the APIs to tell if something has changed, if they need to
be modified. If you're running YAPF as a subprocess, then it depends on the
flags you use. If you use the '--diff' flag, then if there's output there
were changes. If you use '--in-place' then it's a bit more complicated, but
it only updates the file if there were changes...

Adding new files to the yapf repository for this purpose is not going to be

accepted at this time. We may reconsider later.

FWIW, I'm an early adopter of yapf and already contributed some bug
reports and would contribute to the project more in the future. From my
perspective, by adding a standard 5-line file with just some metadata about
how yapf is called that makes it easier to integrate with other tools, yapf
can only benefit by having more people try it out and contribute to it. So
I hope you do reconsider! </$.02>

—
Reply to this email directly or view it on GitHub
#57 (comment).

[asottile]

This doesn't really make sense to me. An exit of non-0 traditionally means that something went wrong (or in the case of "grep" it didn't find anything).

My thought is that like most linters you would return 0 when the code is good and nonzero when the code is wrong (needs reformatting, etc.). Which would mean when running with --diff and there's any diff you'd return nonzero. When running with --in-place it would return nonzero when a change is made.

Does this sound reasonable?