Hi, Robert. I agree with everything you said. Furthermore, I would like to adopt the Fauxton style for the Nano project (and I propose that it extends to all JavaScript under the CouchDB umbrella).
As a first step, I will simply remove the tests from pre-commit hooks. In the future, I would like to re-use or duplicate your CI tools for Nano. But first thing's first! I want to fix a couple of bugs and ship a release. On Wed, Sep 16, 2015 at 7:18 AM, Robert Kowalski <r...@kowalski.gd> wrote: > 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 <jason.h.sm...@gmail.com> > wrote: > > > Hi, Dale. > > > > On Tue, Sep 15, 2015 at 7:26 PM, Dale Harvey <d...@arandomurl.com> > 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.) > > >
