Re: Add an optional HOLD argument to "n" Org macro

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

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