From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2610 invoked by alias); 24 Jun 2017 03:52:21 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: X-Seq: 41353 Received: (qmail 12700 invoked from network); 24 Jun 2017 03:52:21 -0000 X-Qmail-Scanner-Diagnostics: from mail-ua0-f171.google.com by f.primenet.com.au (envelope-from , uid 7791) with qmail-scanner-2.11 (clamdscan: 0.99.2/21882. spamassassin: 3.4.1. Clear:RC:0(209.85.217.171):SA:0(-1.0/5.0):. Processed in 1.532085 secs); 24 Jun 2017 03:52:21 -0000 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2,SPF_PASS,T_DKIM_INVALID autolearn=unavailable autolearn_force=no version=3.4.1 X-Envelope-From: schaefer@brasslantern.com X-Qmail-Scanner-Mime-Attachments: | X-Qmail-Scanner-Zip-Files: | Received-SPF: pass (ns1.primenet.com.au: SPF record at _netblocks.google.com designates 209.85.217.171 as permitted sender) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=brasslantern-com.20150623.gappssmtp.com; s=20150623; h=from:message-id:date:in-reply-to:comments:references:to:subject :mime-version; bh=rYaaJMRUQot6OAMs6GzLIQE3SUH3xkKY29O+3Y7R214=; b=ZrFAaZzze1Xs5f8N7IhZORx7lQDg/PUbTYq86220hqoBpNTbZlHwM1ZQqv+R7k6Zqj Vxz6LNNicwR5e+VuOQARAG5veZT3oAGwSEFSMLq/bVG1P5+w2tGQjeqthBVT6sgQhoUf alGBf098FpVvWw7kj8x98XrpDkmmYso0qRQUmlqvc1FfNdzVA9OaaN2UM/pM57lT4pUm HOg3aLjAf0o0YE32JmuKBplylAQybN9puEG8j9UyEimvJ5y4fTOCqFpqr+yL9KDoPpqN RWLhJOj2lLv6dR+K1dfl9g1HIeGzh1pwE8HeCspXBKECFf9WTxPOffP2uIrQSuIFIGpC +J0g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:message-id:date:in-reply-to:comments :references:to:subject:mime-version; bh=rYaaJMRUQot6OAMs6GzLIQE3SUH3xkKY29O+3Y7R214=; b=WN6DezVOSdyaXHqRfjq9evZqDKZyXJHiN5E9fZD/kz5miQ//E0gsGOY/7VC7K6lyM+ ylXRvMrqcRMlKqbWKILZE0GJlPo5r0zGddqvNDRTQwAnCCvnrd7A90R9vR4AWaXXH+ac IlcJduhm6sUGfRU9D5Bm5Iw4soToCqFUM5XW40yJCY+kWlz1lKs/8HYmOI25Q17sePlg cjYb7aPP1TcxlN6wZnpV31AdrxGPRhvOzKniZudcVKRfIR73dfU+mlgzKlpTrcQQC9fP Xzd/PQ8f5f2wvkeLFpd9VUtFOZnO2FB4gA7W7zkBIZdfIRDtSi3wlicrJ44CTTRktsC3 wydQ== X-Gm-Message-State: AKS2vOxjRMs3NAk/h1bKYIeYIr7r21fZyR996sTUt/EfWPsVUYqHbWC8 gw70VPncXQHnKts9q8w= X-Received: by 10.159.35.136 with SMTP id 8mr8005347uao.1.1498276334258; Fri, 23 Jun 2017 20:52:14 -0700 (PDT) From: Bart Schaefer Message-Id: <170623205312.ZM27285@torch.brasslantern.com> Date: Fri, 23 Jun 2017 20:53:12 -0700 In-Reply-To: <1498253451.3581506.1019483080.3F968E2D@webmail.messagingengine.com> Comments: In reply to Daniel Shahaf "Re: [PATCH] 2 modules, zsh/db, zsh/gdbm, bug-fixes" (Jun 23, 9:30pm) References: <170623013643.ZM7546@torch.brasslantern.com> <1498253451.3581506.1019483080.3F968E2D@webmail.messagingengine.com> X-Mailer: OpenZMail Classic (0.9.2 24April2005) To: zsh-workers@zsh.org Subject: Re: [PATCH] 2 modules, zsh/db, zsh/gdbm, bug-fixes MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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.)