From: Bart Schaefer <schaefer@brasslantern.com>
To: zsh-workers@zsh.org
Subject: Re: [PATCH] 2 modules, zsh/db, zsh/gdbm, bug-fixes
Date: Fri, 23 Jun 2017 20:53:12 -0700 [thread overview]
Message-ID: <170623205312.ZM27285@torch.brasslantern.com> (raw)
In-Reply-To: <1498253451.3581506.1019483080.3F968E2D@webmail.messagingengine.com>
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.)
next prev parent reply other threads:[~2017-06-24 3:52 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-06-22 9:04 Sebastian Gniazdowski
2017-06-22 9:05 ` Sebastian Gniazdowski
2017-06-23 9:29 ` Bart Schaefer
2017-06-23 8:36 ` Bart Schaefer
2017-06-23 21:30 ` Daniel Shahaf
2017-06-24 3:53 ` Bart Schaefer [this message]
2017-06-24 4:56 ` Sebastian Gniazdowski
2017-06-25 0:50 ` Daniel Shahaf
2017-06-26 2:40 ` Sebastian Gniazdowski
2017-06-24 17:36 ` Bart Schaefer
2017-06-25 8:06 ` Sebastian Gniazdowski
2017-06-25 11:22 ` Sebastian Gniazdowski
2017-06-25 22:52 ` Bart Schaefer
2017-06-27 5:46 ` Sebastian Gniazdowski
2017-06-27 8:39 ` Daniel Shahaf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=170623205312.ZM27285@torch.brasslantern.com \
--to=schaefer@brasslantern.com \
--cc=zsh-workers@zsh.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/zsh/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).