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

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Sebastian Gniazdowski wrote on Sat, 24 Jun 2017 06:56 +0200:
> On 24 czerwca 2017 at 05:53:12, Bart Schaefer (schaefer@xxxxxxxxxxxxxxxx) wrote:
> > 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.

I don't think calling out the folding is picky.  It's perhaps a higher
degree of attention to detail than is usually seen on this list, but
it's a valid coding style issue.  (More on this below)

> The folding helps in redis module, where set of GSU objects is devoted to 6 different types.
> 

Two things.

Firstly, I'm not sure why you need the markers at all.  Vim's «:set
foldmarker=syntax» works well and doesn't require any markers.

Secondly, I have no objection to editor-supporting markup added,
provided that it is unobtrusive… which these markers are not.  They get
in the way of reading the code (they look like comments, and people who
read the code read comments).  They're liable to get out of date
(functions get split and renamed), and it won't scale to have every
$EDITOR's style of markers.  So I would rather they weren't added to the
VCS tree.  (but feel free to use them on your own machine, of course)

> > } 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.
> 
> [...] The problem is: gdbm_reorganize doesn't work correctly on NFS
> filesystems. Short test code:
> 
> https://github.com/zdharma/hacking-private/tree/master/gdbm
> 
> Effect on NFS:
> 
> https://asciinema.org/a/ocjBIM9sEcvnNovA9AkStSwsQ
> 

Isn't there a permanent link to a description of the bug?  Preferably in
the gdbm bug tracker, or at least on their mailing list?  That's needed
so people in the future can know why the workaround was added and judge
whether it can be removed.

(Both of these links are susceptible to rot: they may not work in a
few years)

> > 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.)
> 
> Yes me too had hard time deciding on this. I can add some names, no problem.

My reasoning was that every error/warning message should be signed by
its originator.  That's useful when the error is generated a few layers
away from the commmand the user actually invoked:
.
    % f() g
    % g() h
    % h() i
    % i() echo "j not found" >&2
    % f
    j not found
    %
.
where f, g, h are each a separate project.

If we don't know the name of the command or parameter/variable, then we
can at least sign with the name of the module.

Cheers,

Daniel