fixed #228: make exit code customizable to indicate whether files were changed

[reece]

As with others on #228, I was surprised to learn that yapf returns a non-success error code even when it completes successfully. This breaks long-standing convention (that, yes, is broken by other tools also).

The attached patch makes the return code in the case of changed files customizable. By default, 0 is returned when files are formatted according to the effective style, regardless of whether changes occurred or not. Callers may request that another exit code be used to indicate successful execution AND that files were modified. For example, yapf -c 2 recovers the previous behavior to exit with code 2.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[coveralls]

Coverage increased (+0.001%) to 96.963% when pulling 6ac26c9 on reece:master into f4de4c6 on google:master.

[reece]

I signed it!​

[googlebot]

CLAs look good, thanks!

[sanmai-NL]

Question, will your PR result in sensible behavior in case YAPF exits because of an unhandled exception? It appears to make no distinction between an error and any normal result.

[reece]

With respect to errors/exceptions, I believe that the code operates exactly as before. Note that no change was made to run_main(), which is where exceptions are caught and sys.exit(1) is invoked. In the absence of an exception, exit code is determined by the return value of main(), and that's where I made changes.

For example, with a non-existent file and my .bashrc:

$ yapf not_a_file.py  2>&1
yapf: Input filenames did not match any python files

$ echo $?
1

$ yapf ~/.bashrc 2>&1
...
    if [ -d "$BASH_CONF_DIR" ]; then
                           ^
SyntaxError: invalid syntax

$ echo $?
1

The PR is intended to act like this:

• Default behavior is now to have only two intentional exit cases:
  • 0: all files are formatted according to style (whether changes were made or not)
  • 1: exception (as with current behavior)
• Custom exit for modified files (-c i):
  • 0: all files formatted, no changes made
  • 1: error
  • i: all files formatted, changes were made

-c 2 precisely matches current behavior.

The PR is implemented with the default -c 0, which effectively makes the default behavior a special case of the custom exit code.

[reece]

@gwelymernans, @vergult, @carlthome, @sanmai-NL : I'd appreciate comments on this PR re: yapf exit codes.

This PR seems like a good compromise to me: folks who want exit code 0 for success irrespective of whether files were modified get that by default, and folks who to distinguish those cases by exit code get that functionality with a simple flag. Exit error codes are unchanged.

[carlthome]

Great!

[sanmai-NL]

@reece:
Not using a non-zero exit status for normal operation by default is a good improvement in your PR. What remains an issue with your PR, is that non-zero exit statuses should really signal an error and not be modifiable or dynamic at all. The concept of exit status has a conventional meaning, which is why the issue #228 was raised in the first place. Though your approach has of course some practical motivation, it is not a wise strategy in general to deviate from conventions. Somebody's exit status i != 0 really should be anybody's fatal exception.

As I wrote in #228, if we are to provide detailed result information (files changed or not), which I think we are, then it's best to return that as regular, parseable standard output, in line with Unix conventions. It could be as simple as returning { result: 1 } in JSON, if returning a raw text-encoded integer is a concern in terms of interoperability and size guarantees.

In YAPF 0.11.0 --diff and --in-place are mutually exclusive. Perhaps that design can be reconsidered, so that any non-zero text output implies that changes were made when invoked with --diff --in-place. This is easy to test for from a shell script.

[bwendling]

Apologies for my absence.

@reece's and @sanmai-NL's comments are well taken. I'm not entirely sure how best to proceed, so I looked at an already established tool: autopep8. It returns 0 for success and 1 for failure, with no other exit codes used. Despite what I did (and coded) in the past, I think we do need to adhere to that standard. So the issue then becomes to somehow signal to a user what files were modified, if they require that information.

Let me throw in my suggestion for good measure:

• Return 0 if there are no errors, whether formatting occurred or not,

• Return 1 when there are errors, and

• Add a --verbose flag that prints a log of files that were modified. Something like:

  $ ls
  bork.py foo.py
  $ yapf -r -i -v .
  bork.py was modified
  $
  

  (For something read from stdin, we could print modified or some other message.)

This retains the 0 is normal / 1 is error standard that most Unix commands adhere to. It also allows those who want to know which files were modified to get that information without unnecessarily spamming those who don't care about that information. My question then becomes whether this is sufficiently "standard" practice for other Unix tools so that people won't be too surprised by its behavior.

Thoughts?

[sanmai-NL]

AFAIK, that would be very conventional and solve the issue at hand. Nonetheless, such output would also be a bit suboptimal as part of yapf's interface, since printing a list of paths gives strictly less information than the existing feature of diff output does.

Note that the ‘was modified’ text would be redundant, since we're talking about parseable stdout output. If you ultimately wish to list paths instead of the diff then I would propose to cut that out. Also, please make the list 0 byte separated.

[reece]

@gwelymernans: That WFM.

Suggestion: merge this PR as-is since it moves in the right direction. Then, create a new issue to discuss and implement a new feature for listing changed files. That will enable the project to move forward with incremental progress.

Thanks for yapf!

[sanmai-NL]

@reece:
That doesn't sound like a solid process to me, since that means unnecessary interface volatility (first -c, later no more -c). Your PR would then be merged as is, but no one waiting for the improvement would get any benefit before a new release is cut or only by pulling in an unstable revision. Your PR is small and can easily be reworked. Getting it right immediately is doable and better.

[reece]

I meant to keep -c for the foreseeable future.

[bwendling]

@reece: I'm not sure we want to keep the -c option around if we go with what I proposed above. If we do eventually go with a -c option, then I'd like to do it a step at a time. So I agree with @sanmai-NL's suggestion about using the patch without the -c for now. Does that sound good?

@sanmai-NL: Excuse my ignorance, but I'm not 100% sure what you mean by 0-byte separated. Do you have an example? From the link, I gather that it's an encoding issue.

As far as what to print out, we could allow the --diff flag to exist with the --in-place flag, though it's not super parsable. I'm completely open to suggestions on this point. :-)

[sanmai-NL]

@gwelymernans: your example output contained line-ending characters to separate the file paths. If you want to list file paths, please separate them by zero bytes instead. Then Unix utilities (e.g., xargs) can process YAPF output even in the face of source files whose paths contains line-ending characters themselves. The link I gave was only indirectly relevant, it explains using a zero byte separator is not as simple as writing b'\x00' for a project that supports Python 2 (such as YAPF), since that only works in Python 3.

[carlthome]

Status?

[bwendling]

@carlthome I'm okay with going ahead with the initial change where 0 indicates a non-error return (whether the file was changed or not) and 1 indicates some type of error. I want to hold off of adding a -c just yet. And I think the logging output can be another patch.

@reece could you modify your pull request to have just a 0 / 1 return? Also, please update the CHANGELOG file.

[coveralls]

Coverage remained the same at 96.962% when pulling 039c0b5 on reece:master into b8d20bd on google:master.

[bwendling]

Thank you!

[JorisE]

I can see the rationale behind returning a 0 whenever there are no errors, but thinking about a fix for the Emacs plugin yapfify made me realise that the new solution also feels weird. When there are changes the whole corrected file is written to stdout, but nothing is written to stdout when the file contains nothing to be reformatted.

If YAPF were to always write the completomething always has to break somewhere when you make a change like this :). After this changee output, you would only have to check for the exit code before using the output somewhere else. In the current situation you would have to check if anything was actually written to stdout, even if you know from the exit code that nothing went wrong.

Would it be a good idea to always return the corrected file on stdout?

[JorisE]

I just found that YAPF does always return output when it operates on stdin. That means the fix for yapfify is easy, but it still seems strange that the behaviour is different between operating on a file or stdin in this way.

[reece]

but nothing is written to stdout when the file contains nothing to be reformatted.

Really? I agree that yapf should always return something on stdout when given input on stdin, even when unchanged (except in cases of legit errors). That behavior is most consistent with decades of Unix stdio conventions (sed, grep, perl, etc).

I'm not seeing the behavior you cite. Some examples:

(default-3.5) snafu$ yapf <setup.py >s1

(default-3.5) snafu$ diff setup.py s1 | wc -l
121 # ⇒ changes were indeed made

(default-3.5) snafu$ yapf <s1 | diff s1 - | wc -l
0 # ⇒ rerunning on reformatted file produced no changes (and not an empty file)

(default-3.5) snafu$ yapf --version
yapf 0.11.1

Is that consistent with what you see?

[JorisE]

Exactly, luckily I found that YAPF does indeed always write to stdout when given input on stdout.

My confusion stemmed from the fact that it does not return anything on stdout when given a file as input, like so:

joris:~/dev/django$ yapf setup.py | wc -l
93
joris@:~/dev/django$ yapf -i setup.py 
joris@:~/dev/django$ yapf setup.py | wc -l
0

Is that also expected Unix behavior? I honoustly don't know, but it surprised me.

[reece]

Although there are formal specs (like POSIX), a lot of behavior is by convention. The following is entirely my opinion.

I like the following basic templates for programs:

• named file in and out: -i input, -o output,
• named file overwrite: -i input, -b (make backup), -f (force overwrite). E.g., cp, ln
• stdio: <input, >output

Conversely, mixing any of the above themes seems vaguely inconsistent. So, -i input and writing to stdout feels inconsistent to me.

[bwendling]

Re the output, @JorisE, you found the answer I was going to give you. :-) (Thank you for patching yapfify!)

As @reece mentioned, the behavior I've seen has been mostly by convention as well. I do want to follow the "least surprise" axiom in this situation. For other formatting tools, like clang-format, I don't believe that they output anything when you use their version of the -i flag, and I don't really think that's the way to go here. However, other tools may allow you to output a diff at the same time. (This is something we don't currently allow, but there's probably not a compelling reason not to allow it.)

I like the idea of -o and -b flags.

[JorisE]

Thanks for the answers.
Just for reference, I found that gofmt -w also never outputs anything (change or no change).

[vergult]

@reece sorry for answering late. I'm totally satisfied with your final changes: small and simple. This will result in a less surprising behavior, thank you! Regarding the -c option, I also find it is better to not include it even though your concern is perfectly legitimate. This change will indeed certainly surprise people the other way around and break operations based on the now old exit logic strategy. My thought is that this aspect would have probably justify pushing the change more in a new major version rather than a minor one. But hey, I perfectly live with the change being already available 😀

Thanks to all!