[DBIx-Class-Devel] Topics for next AllHands on 2012-12-04 22:00:00 (UTC) irc.perl.org#dbic-cabal

[Brendan Byrd]

On Mon, Dec 3, 2012 at 6:37 AM, Peter Rabbitson <rabbit+dbic at rabbit.us> wrote:
>
> I believe a lot of the friction comes from this point, which jnap also
> mentioned: there is no agreement on any of the following points wrt the
> DBIC project:

Forgive my replies for AllHands topics here, but certain thoughts work better in
email vs. chat.  Besides, some pre-meeting discussion is a healthy thing.

> * Minimum requirements for code changes (architectural considerations,
> maintainability, performance considerations, backwards comp.
> considerations)

I think that the test platform should be able to flush that out.  If
the code isn't sane,
it shouldn't pass the test platform.  And if it DOES pass the test
platform, then
the test platform should have new tests.  Granted, there will some portability
issues (ie: the coder is using Linux, and it fails in Windows), but we have
enough testers to root those out.

Although, I gather that you're talking more about new features and
functionality, which can get out of control in the wrong hands.  That's a
philosophy argument, and we'll likely need a starting point to shoot holes into.

> * Minimum requirements for documentation changes (agreement with
> existing code, link quality, formatting considerations)

DBIx::Class::Manual::Reading?  If that isn't the appropriate place, then we
should adjust the language "read and write DBIx::Class POD" in the
title.

> * Minimum requirements for patch density (multiple unrelated changes
> lumped in one patch, general review considerations)

I'll speak to my experiences because I am probably the biggest offender
to the "norm" of these kind of changes.  I'd been new to git and multi-coder
projects in general.  Here's my take (ie: what I've learned):

* Use the standard "present tense" for the commit log: "Verb thingy", etc.
* Multiple items should have a header at the top, blank line, followed by the
multiple items.
* New code changes should be in a topic/* branch.

As far as what is considered unrelated, in -general-, topic branches should
be one commit and one branch with one type of change.  However, this
always isn't 1:1 for several reasons:

* The topic should be a bit more global than a single commit.  The commit
goes into code-related specifics to the topic you are trying to cover.

* A bunch of smaller changes can work better as a single commit.  If it's
readable as a commitdiff, it's probably better to combine as one.

* Some changes will fall into dependencies that you can't separate as two
branches.

In the case of my 'topic/docs-pod-inherit', I kept the first commit to save
on the amount of re-reading of commitdiffs, since it was a large change.
Also, there were some changes that were simple to understand, but
involved a lot of file changes, which were separated out to different files.

In general, the topic/commit guidelines should be considered guidelines.
Granted, I've done some bang up jobs combining too much at once, but we
also shouldn't be nitpicking with "The Rules" for these.  Not fraking with
master?  Yeah, that's a pretty hard rule.  Putting two items into two
commits instead of one?  Meh, only when it's unreadable otherwise.

This kinda leads into a discussion about politeness about the
patcher/developer relationship, or even the developer/pumpking
relationship.  There should be a happy medium struck:

The developer should be polite that somebody actually wants to improve
the codebase and, instead of just demanding features, is actually *gasp*
submitting real code.

The patcher should be polite that the developer needs to review it, clean
it up, and possibly write tests, if they aren't included.

-- 
Brendan Byrd <Perl at ResonatorSoft.org>
Brendan Byrd <BBYRD at CPAN.org>