From mboxrd@z Thu Jan  1 00:00:00 1970
From: Nicolas Goaziou <mail@nicolasgoaziou.fr>
Subject: Re: Add an optional HOLD argument to "n" Org macro
Date: Thu, 15 Jun 2017 18:07:28 +0200
Message-ID: <871sqli69r.fsf@nicolasgoaziou.fr>
References: <2ee94a64a94b46259b0da6e7d34675c9@HE1PR01MB1898.eurprd01.prod.exchangelabs.com>
	<87y3u7o3dj.fsf@t3610>
	<a4c6d561b12a4cc8ad4fe8c017fa2121@HE1PR01MB1898.eurprd01.prod.exchangelabs.com>
	<87pofjtk4b.fsf@t3610>
	<2069df8c23bc43f3b04b6e203b96be9d@HE1PR01MB1898.eurprd01.prod.exchangelabs.com>
	<87r2zvpyst.fsf@delle7240> <8760guib5i.fsf@nicolasgoaziou.fr>
	<87fuftb4lg.fsf@nicolasgoaziou.fr>
	<CAFyQvY1cLtMXFgNh0R-Fv_hg+8TWGEGe_NKLebPykk=rkevCiw@mail.gmail.com>
	<CAFyQvY0+rLM+kKTqHGNuMnL9asmafnLYqLJ97UOGqLaXuFmbDw@mail.gmail.com>
	<CAFyQvY19R4gEkEP1pBwvVguyC081u2qn5WuDHkC_mGv_S_5YrQ@mail.gmail.com>
	<8760fyic6n.fsf@nicolasgoaziou.fr>
	<CAFyQvY133N0k1K3A_Gc2b9fP4MrRMcco=WwzayD3mG6R==BALg@mail.gmail.com>
	<CAFyQvY3EmjqoFBYu_PkiGTJoNzt_ZW7pwRCTVCMZh24MSNxJQQ@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain
Return-path: <emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org>
Received: from eggs.gnu.org ([2001:4830:134:3::10]:41576)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <mail@nicolasgoaziou.fr>) id 1dLXJ3-0007Ob-7o
	for emacs-orgmode@gnu.org; Thu, 15 Jun 2017 12:07:38 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <mail@nicolasgoaziou.fr>) id 1dLXIz-0007XO-9M
	for emacs-orgmode@gnu.org; Thu, 15 Jun 2017 12:07:37 -0400
Received: from relay4-d.mail.gandi.net ([217.70.183.196]:42500)
	by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)
	(Exim 4.71) (envelope-from <mail@nicolasgoaziou.fr>)
	id 1dLXIz-0007XF-2o
	for emacs-orgmode@gnu.org; Thu, 15 Jun 2017 12:07:33 -0400
In-Reply-To: <CAFyQvY3EmjqoFBYu_PkiGTJoNzt_ZW7pwRCTVCMZh24MSNxJQQ@mail.gmail.com>
	(Kaushal Modi's message of "Thu, 15 Jun 2017 15:25:42 +0000")
List-Id: "General discussions about Org-mode." <emacs-orgmode.gnu.org>
List-Unsubscribe: <https://lists.gnu.org/mailman/options/emacs-orgmode>,
	<mailto:emacs-orgmode-request@gnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/archive/html/emacs-orgmode/>
List-Post: <mailto:emacs-orgmode@gnu.org>
List-Help: <mailto:emacs-orgmode-request@gnu.org?subject=help>
List-Subscribe: <https://lists.gnu.org/mailman/listinfo/emacs-orgmode>,
	<mailto:emacs-orgmode-request@gnu.org?subject=subscribe>
Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org
Sender: "Emacs-orgmode"
	<emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org>
To: Kaushal Modi <kaushal.modi@gmail.com>
Cc: emacs-orgmode@gnu.org

Hello,

Kaushal Modi <kaushal.modi@gmail.com> writes:

> On Thu, Jun 15, 2017 at 9:10 AM Kaushal Modi <kaushal.modi@gmail.com> wrote:
>
>> The patch based off latest master is attached. Please review.

Thank you. Some comments follow.

> This patch adds a dependency on subr-x library for string-trim function
> that was added in emacs 24.4.

We do not need this dependency. In particular, there is already
`org-trim'.

> * lisp/org-macro.el (org-macro--counter-increment): Rename the
> optional arg RESET to ACTION, as now that action can mean setting,
> resetting or even holding the specified counter.  ACTION set to
> "hold" or "-" will hold the previous value of the counter.

It is confusing to provide two ways to achieve the same action. I'd
rather have "-" only.

> +Any other non-empty string resets the counter to 1."
> +  (let ((action-trimmed (when (org-string-nw-p action)
> +			  (require 'subr-x)
> +			  (string-trim action))))

See above.

> +  ;; Second argument set to "-" or "hold" holds the counter value.
> +  (should
> +   (equal "1.1 2.2 8.3 8.1 8.2 8.3 9.3 9.3"
> +          (org-test-with-temp-text
> +	   (concat "{{{n(,-)}}}.{{{n(c)}}}" ;Hold before even starting the counter
> +		   " {{{n}}}.{{{n(c)}}}"    ;Increment after hold
> +		   " {{{n(,8)}}}.{{{n(c)}}}"
> +		   " {{{n(,hold)}}}.{{{n(c,reset)}}}" ;Alternative hold arg
> +		   " {{{n(, - )}}}.{{{n(c)}}}"	      ;With spaces
> +		   " {{{n(, hold )}}}.{{{n(c)}}}"     ;With spaces
> +		   " {{{n}}}.{{{n(c,hold)}}}" ;Hold on another counter
> +		   " {{{n(,hold)}}}.{{{n(c,-)}}}") ;Hold on both counters
> +           (org-macro-initialize-templates)
> +           (org-macro-replace-all org-macro-templates)
> +           (buffer-substring-no-properties
> +            (line-beginning-position) (line-end-position))))))

Could you split this into smaller tests, each one testing one feature?

Regards,

-- 
Nicolas Goaziou