Hey there, just as a report how we are doing it with Fauxton:
the Fauxton project also has a styleguide in order to make reviews and reading code easier for everyone. At one point it turned out to reduce a lot of work for the reviewers if we test automatically for the coding style as part of our testsuite. We don't use commit hooks for it, but the CI will fail if the automatic coding style check and also the testsuite was not successful. the style check is runs before the unit tests run, which run before the integration tests. Red tests (that include tests for style) are a no-go for a merge in Fauxton. I don't think the checks must be enforced on a pre/post-commit-hook-level but somewhere they should notify the proposer of the change to make the live of the reviewer easier - in case you think code style is important for your project. I also want to not that some sub projects of CouchDB have no styleguide at all and that it can be a positive thing or a negative thing. Having one worked out well for Fauxton. I also think that deploying with a red CI might be OK in exceptional cases, but in these cases the deploying person should be aware why the test fails in 100%, and know the reasons why it is not fixable right now. In the recent past I have seen people saying they would know why the integration tests fails, delivering buggy releases. I think it is a small boundary and not everyone who says they know why the tests fail is 100% sure why they really fail (and deploys broken releases therefore). But I have worked with people that were so deep into the testsuites and the project they maintained in the past that they were able to predict that. In short I support whatever you decide for I just wanted to share my experiences, maybe it helps :) On Tue, Sep 15, 2015 at 2:54 PM, Jason Smith <[email protected]> wrote: > Hi, Dale. > > On Tue, Sep 15, 2015 at 7:26 PM, Dale Harvey <[email protected]> wrote: > > > > > I just wanted to clarify, are you speaking about removing as a > "pre-commit > > hook", or removing the requirements for those checks to pass before > > merging? > > > > I am speaking about removing the pre-commit hook only--the mechanical thing > that that runs automatically when one commits. > > The tests and checks would make brilliant push hooks, or perhaps Travis > tests for pull requests. However they are a bit much as a *commit* hook. > > A separate conversation: should the tests pass for merging. I would say: > yes if it's smart; no if it's dumb: they need not pass for merging, at > least not automatically, mechanically. The reason is that we should merge > pull requests liberally, accepting contributions from all, then commit on > top of those to correct for style. I won't have some sort of angry > Calvinist robot telling me I can't merge a pull request. (If we can be > clever, for example to require all tests pass for the master branch but not > feature branches, then yes I would love that.) >
