yapf returns an exit code of 2 on unformatted but valid input.

[vergult]

Hello there and thanks for developing this much appreciated python formatter.

I’m opening this issue as I’m not sure to understand the logic behind yapf's exit code return when applying it on unformatted and valid input. Indeed, yapf returns an exit code of 2 in such case (which implies something went wrong), yet a valid formatted output is returned. I would consider the operation successful, hence expect an exit code of 0.

Consequently, this behavior tends to break scripts where flow execution rely on the format's success.

I saw a related issue: https://github.com/google/yapf/issues/186 but I don't see the need to inform output has been modified compared to input (i.e. formatted) by returning a status code other than 0 as formatting is the purpose of the tool. Otherwise, _--diff_ option seems the right fit if this information is needed (not to mention UNIX tools can also answer this).

Am I missing something or is this behavior indeed odd? Thanks!

[jamesbroadhead]

This is a really useful feature if you're writing a CI job to assert that the patch has already been formatted -- make assert_formatted.
It's also useful in a pre-push hook -- "the patch isn't formatted already, so commit the format changes, then push again"

[gwelymernans]

The reason for the bizarre exit codes is due to yapf trying to indicate three different states:

1. A return code of 0 indicates that nothing was formatted and the program didn't have an internal failure.
2. A return code of 1 indicates that the program had an internal failure. It's this way because Python apparently returns 1 in such cases.
3. A return code of 2 indicates that some files were changed.

The request in #186, if I remember correctly, was due to them wanting to use yapf in a scripting environment. In that case, it would involve testing $? to see what value it has. I'm not sure how else to indicate these three different states effectively...

[vergult]

Thanks for your replies.

I've actually opened this issue as I frequently apply shell commands and/or scripts when editing a file's content. Those scripts are typically formatters when dealing with source code and for python files, I use yapf. Sanely, when applying a script on a file's content which returns a non zero exit code, my text editor assumes something went wrong, so it doesn't modify the file in place and outputs an error. Consequently, when I started using yapf, I always had errors when formatting valid but unformatted python files. I found that annoying as it was defeating my purpose of using a formatter. Eventually, I created a simple wrapper script which maps a yapf exit code of 0 or 2 to and exit code of 0. Therefore, my personal issue is fixed.

I just thought I would notify this exit behavior I find a bit broken. Indeed, the main goal of the tool is to format python source code, so let it format it and notify the operation's success or failure. I mean by that, whether the file was previously formatted or not, the expectation is that the file is formatted at the end. Hence, unless the input was invalid from the start, formatting is to be considered successful from my point of view.

I do understand @jamesbroadhead concerns but would probably not deal with it at the formatter level (not for the hook case at least).

The tool could possibly deal with most use cases while having a better primary usage exit code logic. This possibility could be to allow combining the '--in-place' option with the '--diff' one. In this case, a zero exit status code could be returned to indicate the file is formatted (whether it has been modified or not) and output the difference between both previous and new versions. This way, inspecting the output would inform on the file's modification state and even give details about it.

I can also perfectly live with my current wrapper script, so feel free to consider my proposition or not ;)

[sanmai-NL]

I think the proper solution is actually simple.
Only exit with non-zero status if an error occured.
Print non-machine parseable output to standard error.
Print a machine parseable result to standard output. This result may have e.g. the semantics currently signaled with the exit status (changes applied or not, etc.).
This machine-parseable output would be so simple in this context that automated builds can still be made to fail, trivially, based on the type of result.

[gwelymernans]

Thanks for you comments! This is a long weekend, so I may not be able to think about this issue until next week. Though patches accepted if you want to cobble together a proposal. :-) I just need to make sure that all parties are satisfied with whatever solution we come up with...

[carlthome]

This confused me too. I'd really prefer exit code 0 when no formatting occurred. It's not an error.

[gwelymernans]

@reece created #286 to address this. I'm not convinced that it's the best way to go though. It appears that people need a more default behavior that's consistent with current tools' behaviors.

@reece, would you be interested in implementing what @sanmai-NL suggests?

[reece]

@gwelymernans I think that proposal requires discussion first.

Issues/points to discuss:

• yapf currently operates on stdin/out. What's the behavior when operating on stdio?
• Whatever change we make, callers are going to have to change how they invoke yapf if they want to distinguish subcases of success (i.e., with and without modified files). With this patch, -c 2 recovers current behavior if that's what people want. With @sanmai-NL's suggestion, callers will end up grepping (or writing wrappers to do so). If I wanted what they're asking for, I'd take the -c option and move on.

Obviously, I'm new to this project. However, I suggest that the core team reconsider whether file tracking is yapf's responsibility at all. I think it's not, and that trying to do so creates unnecessary complexity that confuses users.

All code that I care about reformatting is under source control. That's probably true for most users. My own workflow is to ensure that a repo is clean, then reformat, test, and commit. Handily, commits on unchanged repos are no-ops, so I don't even have to check whether there really were changes. (BTW, this is all in a makefile, for which unconventional exit code meanings causes real problems, and that's why this exit code business matters to me at all.)

[sanmai-NL]

@reece: for discussion on your PR, let's continue at https://github.com/google/yapf/pull/286#issuecomment-236133579.

[reece]

@sanmai-NL: Good call. I'll copy my response there for discussion.