[GitHub] [geode-native] pdxcodemonkey commented on issue #558: GEODE-7520: Disable clang-format by default

["pdxcodemonkey (GitHub)" <gi...@apache.org>]

I respectfully disagree:
* Just as a ballpark estimate, developers on the team spend several minutes of build time per day running clang-format on code.  This doesn't strictly need to be done until it's time to push.
* In point of fact, formatting is *not* enforced in the build, or at least not completely.  _Running_ the formatter is enforced, but committing the changes is not, and can't be without the use of a Git hook or the moral equivalent.  As a result, PRs fail in Travis CI due to formatting with some regularity.  This change doesn't materially change the situation w.r.t. enforcement.
* Formatting is not alone in this situation, either.  We don't enforce RAT checks or LGTM prior to submitting a PR, but errors will fail Travis and block merging.
* Having a single target to format the entire codebase is much simpler than adding a format dependency to arbitrarily many targets in the tree.  Running clang-format on everything in a CI job currently involves a grotesque single-line sed script piped through xargs, running `cmake --build . --target clangformat-all` is a huge improvement on this.
* Just philosophically, having an explicit target so that you can turn on or off is preferable to the implicit criteria "you don't get any clang-format targets if you don't have the clang-format executable on your system".  With the current setup, if someone new to the project doesn't have clang-format on their system, how are they to know that's a problem?  OTOH, if they attempt to build the clang-format target and get a "target not found" error, it's more obvious what's up.

I'd be open to a solution that allowed this target or targets to be on by default if they can a) be explicitly turned off via cmake variable and b) run for the entire code base without doing backflips on the command line.  Let me know what you think.

[ Full content available at: https://github.com/apache/geode-native/pull/558 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org