Integration as a http://pre-commit.com/ hook about yapf HOT 11 CLOSED

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?

from yapf.

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. :-) )

from yapf.

@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.

from yapf.

@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.

from yapf.

@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.

from yapf.

@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.

from yapf.

@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

from yapf.

@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).

from yapf.

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.

from yapf.

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>

from yapf.

On Wed, Apr 1, 2015 at 11:17 AM jab [email protected] 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).

from yapf.