Re: [PATCH] 2 modules, zsh/db, zsh/gdbm, bug-fixes

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

On Jun 23,  9:30pm, Daniel Shahaf wrote:
}
} I assume the patch adds a zsh/db module that introduces abstractions
} that all (existing and future) zsh/db/* modules can share?

Yes, that was always the intent behind db_gdbm.c -- to be a model for
adding more databases later.

} If (user or dev) documentation needs to be written, I would prefer it
} were written _before_ the patch was accepted.

Agreed; Sebastian has taken the approach of writing extensive usage
messages in the commands themselves rather than editing yodl.  The
yodl doc should be updated.

} Is it a problem that the module name is a path-wise prefix of another
} module's name (zsh/db v. zsh/db/gdbm).

No, it is not an problem.  The whole business with slashes is just a
convention anyway, like using colons in zstyle contexts; the underlying
mechanism doesn't distinguish.

} There's a ton of vim folding added, {{{ }}}.  We don't have it
} elsewhere, I assume it's just a scalpel.  Likewise with the
} whitespace (indentation) changes.

Yes, I would also prefer the vim folding be removed, and the use of
CamelCapsVariableNames be made more like the rest of the code, but
both of those seemed unnecessarily picky.

} The #if 0 looks alarming.

It's around code that was never present in db_gdbm.c before, and protects
a function call that's "new" (in a historical sense) to the library.  I
think it's also nothing but an optimization, and therefore not essential.

} surprised if the correct response to a bug in some configurations

It's not a bug, it's a new(er) feature.

} db.c has variables declared in the middle of the block.  That's a
} C99-ism, and while db/gdbm.c does it, the rest of zsh does not.  Not
} critical.

There are also C++ / C99 style comments, which should be removed.  I
forgot to mention that in my first message.
 
} db.c has a couple of zwarn() that don't state the origin of the error,
} I think it'd be useful to make them zwarnnam(name="zsh/db") or so.

I've had a hard time deciding which of the warning functions is the
right one to use in a given situation.  zwarnnam() is usually meant to
prefix the message with the name of a command that caused the error;
it's not obvious that it should be used in a context where there is no
command involved.  (I haven't dug into whether that is the case for
the instances you've called out.)