couchdb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Robert Kowalski <...@kowalski.gd>
Subject Re: A Plan: Remove pre-commit, jshint, code style requirements from couchdb-nano
Date Wed, 16 Sep 2015 00:18:58 GMT
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.smith@gmail.com>
wrote:

> Hi, Dale.
>
> On Tue, Sep 15, 2015 at 7:26 PM, Dale Harvey <dale@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.)
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message