Hi all,
I've been spending a little bit of time thinking about the past few
days, when we frantically reviewed patches and fixed bugs in advance of
today's demo. Since having a code freeze ahead of the actual deadline is
something relatively new for us, I don't necessarily see it as a
tremendous failing that we didn't nail the execution on the first try,
but I've been thinking about the places I could do better next time. I
share these musings here hoping to start a constructive discussion.
One thing is that I'd like to get into some sort of regular habit of
reviewing patches on the mailing list. This sprint, I mostly thought,
"I'm too busy writing code to bother with reviewing patches," which, in
hindsight, was rather selfish to those who DID take time to review
patches, and to those who needed patches approved. What I think I'd like
to do is try to get into some sort of rhythm, where I maybe set aside 30
minutes each day to review patches on the list. It occurs to me that an
added bonus of reviewing patches is that I get a better sense for what
other people are doing on the project.
(As long as I'm publicly admitting my faults, I'd also like to stop
using the morning scrum reports to blindly dump my list of tasks, and
actually look at what others are working on. That, of course, is what
I'm meant to be doing anyway, but it's easy to get in the habit of just
posting my status and moving on.)
I'd also like to propose that we make a little more use of the Doctrine
of Triviality. (I probably made that term up.) It seems like it's been
acceptable for a long time to push trivial changes without sending them
for review -- making a one-line change, reverting a broken commit, and
so forth. While I am NOT proposing that developers push patches adding
new features or making large changes on their own, there were a handful
of patches on the list that were extremely trivial, or that were just
obviously correct. Some of them sat for days waiting for someone to
review them. I think it would be okay for people with commit access to
push these things on their own, but note that they have a very high duty
of care to make sure they're well-tested. How do other people feel about
this concept? (For example -- I'd like to add some README files to the
projects, just a quick overview mostly for the benefit of the GitHub
pages. I sent one for Conductor some weeks ago that was ignored. I think
it'd be fine to just push things like that.)
As long as I'm proposing changes, I'd like to argue that we should
consider test failures on master unacceptable, and revert any patch that
introduces them. Checking in code with failing tests ends up causing
huge headaches for other developers. For one, it prevents the demo
server / Jenkins suite from running to completion. Second, it means
that, for me to run tests on a patch, I need to run them twice -- once
on the patch, and once on master, to make sure that the test failures I
get aren't new failures in what I'm testing. What I'd propose is that,
if code on master causes any test failures, we just revert the offending
patch as soon as it's identified. (Of course, as with most of my other
points, I don't mean to imply for a second that I'm not guilty of this
myself.)
In any event, I do think we're making great progress, and I'm really
glad to be a part of this team. But I figured that if I was thinking
about these things, other people probably were, too, so I hope this is
the start of a constructive discussion. Please let me know what you
think, good or bad.
Best,
Matt