Re: [PATCH 14/25] Fix old style notmuch-fcc-dirs configuration check.

Subject: Re: [PATCH 14/25] Fix old style notmuch-fcc-dirs configuration check.

Date: Fri, 03 Jun 2011 13:05:00 -0700

To: Dmitry Kurochkin, Notmuch Mail

Cc:

From: Carl Worth


On Thu, 02 Jun 2011 10:49:57 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Well, it says that changes are in notmuch 0.5.  So "old" and "previous"
> refer to pre-0.5 (i.e. 0.4) and "new" refers to 0.5.

Sure, but I happen to ahve already forgotten the details of how the
variable could be configured in 0.4 and in 0.5. More importantly, anyone
in the future reading the commit log is much more likely not to
remember.

> Any configuration when `notmuch-fcc-dirs' is a list.  That variable has
> a nice documentation.

Again, I'd like our commit messages to be self-contained. They are much
more useful if the describe the change being made without assuming to
much outside knowledge.

> > It would be easier to understand the code if there were a corresponding
> > test case for it.
...
> I do not think we need a test for this fix.  What we need are tests for
> FCC functionality when notmuch-fcc-dirs is a list.

Yes!

> Old configuration format was changed in 0.5 in an incompatible way.
> There is a check for the unsupported old-style configuration.  But the
> check is broken and results in an error when running with a valid
> new-style configuration.

This is actually what I meant by "corresponding test case". If the bug
here is that a "new-style configuration" doesn't work , (and I still
don't like that wording---don't say "new style"---explain what it
actually *is*), then yes, we need a test case showing that bug.

> I am not sure what you expect from the commit message here.  IMO it is
> enough for this small bugfix and those who interested can always refer
> to documentation for details.

The commit message should provide a self-contained description of the
change. It should be along the lines of:

	When fcc-dirs is set to
	<some-particular-datatype-that-should-work> notmuch was
	incorecctly detecting this as the
	<old-style-that-is-no-longer-supported> and generating an error
	message. Fix the test so that this configuration now works.

Where the <phrases> above should be replaced with actual descriptions,
not relative pointers to information like "old style" or "new style".

Does that make sense?

-Carl
part-000.sig (application/pgp-signature)

Thread: