Yapf: Exit code with diff

[retr0h]

Diff returns an exit code of 0 regardless. This doesn't seem ideal since CI scripts can no longer rely on a exit code. I would expect diff to return a non-0 if it were to make a change. I understand #228 introduced this change, and I agree a 0 should be returned when making a format change, but running a diff is an entirely different use case, and IMO we should error when the files would be changed.

[retr0h]

I see the exit code is customizable. For some reason I don't have that CLI option.

[jodewey:~] 2 % yapf --version
yapf 0.13.1
[jodewey:~] % yapf --help
usage: yapf [-h] [-v] [-d | -i] [-r | -l START-END] [-e PATTERN]
            [--style STYLE] [--style-help] [--no-local-style]
            [files [files ...]]

Formatter for Python code.

positional arguments:
  files

optional arguments:
  -h, --help            show this help message and exit
  -v, --version         show version number and exit
  -d, --diff            print the diff for the fixed source
  -i, --in-place        make changes to files in place
  -r, --recursive       run recursively over directories
  -l START-END, --lines START-END
                        range of lines to reformat, one-based
  -e PATTERN, --exclude PATTERN
                        patterns for files to exclude from formatting
  --style STYLE         specify formatting style: either a style name (for
                        example "pep8" or "google"), or the name of a file
                        with style settings. The default is pep8 unless a
                        .style.yapf or setup.cfg file located in one of the
                        parent directories of the source file (or current
                        directory for stdin)
  --style-help          show style settings and exit; this output can be saved
                        to .style.yapf to make your settings permanent
  --no-local-style      don't search for local style definition

[gwelymernans]

I believe that was reverted.

[retr0h]

Ah, yes, you're right. The question still remains. A 0 exit code on diff seems incorrect, when diff is likely consumed by scripts in most cases.

[rgreinho]

I am facing the same issue.

As a workaround, I created a wrapper for YAPF which behaves like you described:

#!/bin/bash

DIR=${1:-.}
LINES=$(yapf -d -r "${DIR}" | wc -l | tr -s ' ')
exit ${LINES}

[nvgoldin]

:+1: - it would be much better to customize the exit code for my usage case, or at least exit with something different than 0. this was the behaviour before, no?

[destijl]

This broken our presubmit style check which expects a non-zero return when there's style problems that need fixing. I think it should operate the same as GNU diff:

Exit status is 0 if inputs are the same, 1 if different, 2 if trouble.

[fletom]

+1

[infinitewarp]

+1 bump

Any further thoughts on this? I'd also love to use yapf --diff in my CI chain, and I'd rather not have to wrap it in another script just to set a proper non-zero exit code.

[Huarong]

Hope this can be implemented

[gwelymernans]

Sorry for leaving this for so long. It looks like there's a general consensus for returning non-zero for a diff. I like the idea of operating as GNU diff does if only for the -d flag. I'll go ahead and make the change.

[gwelymernans]

See:

To https://github.com/google/yapf
010a201..eeb6d46 master -> master

Please let me know if this fixes the issue.

[Huarong]

@gwelymernans Works great. Thank you.

[gwelymernans]

Great! :-)