From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.4 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 22079 invoked from network); 15 May 2023 09:05:02 -0000 Received: from zero.zsh.org (2a02:898:31:0:48:4558:7a:7368) by inbox.vuxu.org with ESMTPUTF8; 15 May 2023 09:05:02 -0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=zsh.org; s=rsa-20210803; h=List-Archive:List-Owner:List-Post:List-Unsubscribe: List-Subscribe:List-Help:List-Id:Sender:Message-ID:Date:Content-ID: Content-Type:MIME-Version:Subject:To:References:From:In-reply-to:cc:Reply-To: Content-Transfer-Encoding:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID; bh=ak9D6Su7jQ7XiqRStP90/wE+AWC1+U5bXKwGrZzAD24=; b=kFitUbIB3BTp4JbHDpLGLA8MAv YVKDPGjwUTUeST9g2LQOLG7mybrWN/KHUMEyeaA51QzZsVAQD0Taq1m7lPLQVrPLsJbe2t0Z1OiAw zoVFymz3IZqba9KKdoTaUK5yieKMniMgTptwh6kJGaXXB1AzVSjbivfYUPV06Y8enlxp026+pw04Z CXNpwFr2iBO5a55G7li5nL3ZDfijxWT0b9XqwDmQPpthIGnuGrWMfEA7MjLEKS8q2KV9hwxsT+b12 Xxevf54c5DKq3wLsELc3pKqZ8GVd26SNl9pagfrR6M20NSicMz8vJDe+QiT5cYVwVRVfr/Jv8FG/Z uje6BzIA==; Received: by zero.zsh.org with local id 1pyU8a-000Juv-JP; Mon, 15 May 2023 09:05:00 +0000 Received: by zero.zsh.org with esmtpsa (TLS1.3:TLS_AES_256_GCM_SHA384:256) id 1pyU7x-000JbC-UX; Mon, 15 May 2023 09:04:24 +0000 Received: from [192.168.178.21] (helo=hydra) by mail.kiddle.eu with esmtp(Exim 4.95) (envelope-from ) id 1pyU7x-000Mqw-5U; Mon, 15 May 2023 11:04:21 +0200 cc: zsh-workers@zsh.org In-reply-to: <20230505114154.76547-2-marlonrichert@users.noreply.github.com> From: Oliver Kiddle References: <20230505114154.76547-2-marlonrichert@users.noreply.github.com> To: Marlon Richert Subject: Re: [PATCH 2/3] Make dynamic dir completion easier to implement MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-ID: <87848.1684141461.1@hydra> Date: Mon, 15 May 2023 11:04:21 +0200 Message-ID: <87849-1684141461.166701@hHTP.8k7X.OiI5> X-Seq: 51744 Archived-At: X-Loop: zsh-workers@zsh.org Errors-To: zsh-workers-owner@zsh.org Precedence: list Precedence: bulk Sender: zsh-workers-request@zsh.org X-no-archive: yes List-Id: List-Help: , List-Subscribe: , List-Unsubscribe: , List-Post: List-Owner: List-Archive: Sorry for not reviewing this sooner. Thanks for following up to Aaron's reply with a bit more detail as that does help. On 5 May, Marlon Richert wrote: > > In the pre-patch example, the recommended way of implementing this was > as follows: > _wanted dynamic-dirs expl 'dynamic directory' compadd -S\] ... > > However, this is problematic for the following reasons: > - The proper place for handling _wanted or other tag logic is in the > completion function _dynamic_directory_name, not in each dynamic dir > name function separately. That's questionable. Adding a tag loop outside as in your patch probably makes it easier for someone to setup dynamic directories. It does simplify the example. And it doesn't stop people using their own tag loop - that can be necessary where the dynamic names are broken down into components for the purposes of completion. So I've no objection to the change. There are aspects of having nested tag loops that isn't ideal but it's better than no tag loop. > - The naming styles of the tag and group are not consistent with those > used for non-dynamic named directories, namely 'named-directories' and > 'named directory', respectively. That's specific to the example in the documentation. For many real cases, the text used for dynamic directories can be more usefully described in some other way. Such as 'recent directory index' in the cdr example. > - The example always inserted a `]` suffix, even if the suffix was > already present on the command line, leading to a double `]]` on the > command line. Yes, that's not good. And passing the suffix to the function as a parameter is a good approach to take. > -local func > -integer ret=1 > +if (( $#dirfuncs )); then > + local -a expl=() > + local -i ret=1 > + local func= There's no need to explicitly assign empty values. The function isn't even relying on the initial value and local does reset the value to be empty anyway. > + local tag=dynamically-named-directories > + [[ -z $ISUFFIX ]] && > + local suf=-S] This will only declare the variable suf local if the condition evaluates to true. If it is false, we could be picking an unwanted value and pass it to compadd. Aside from that, I don't think the condition is correct because the user could be completing in the middle of a word. zsh_directory_name_cdr uses [[ $ISUFFIX = *\]* ]] || addsuffix='-S]' Within _dynamic_directory_name, I think it should be just [[ $ISUFFIX = \]* ]] > -if [[ -n $functions[zsh_directory_name] || \ > - ${+zsh_directory_name_functions} -ne 0 ]] ; then > - [[ -n $functions[zsh_directory_name] ]] && zsh_directory_name c && ret=0 > - for func in $zsh_directory_name_functions; do > - $func c && ret=0 > + _tags "$tag" > + while _tags; do > + while _next_label "$tag" expl "$descr"; do > + expl+=( $suf ) You can simply add $suf to the end of the call to _next_label and it will add extra options to $expl: while _next_label "$tag" expl "$descr" $suf; do The cdr functions that Bart mentioned need their own descriptions but at least zsh_directory_name_cdr could be tweaked to take advantage of the suffix now being passed to it. I don't see this breaking any existing dynamic directory functions which was my initial worry before looking at the changes more closely. Oliver