Re: Allowing multiple date trees in a single file

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

Carsten Dominik <dominik@uva.nl> writes:

> We do disagree here a bit.  This little bit of extra work just keeps the
> existing templates working.  We do not introduce a really different
> structure of the org-capture-templates.  Rather, the code introduces a new
> target type, and it makes some older target types be implemented as special
> versions of the new ones.  The old targets are no longer in the manual, any
> customize user will be switched automatically.  What remains is a small bit
> of code that makes sure that the setup of user who might have been using
> that for a long time continues to work.
>
> In my eyes, this is worth it.  Breaking something in a new version is no
> big deal for people who use Org regularly, but I am sure there are a lot of
> users out there who have not changed their setup for a long time, have not
> followed the discussions here and would be frustrated if their setup breaks
> after getting a new version of Emacs, for example.  So we can shoot a
> warning, but I would vote for just keeping this piece of code
> indefinitely.

Our sole disagreement is above duration. "forever" is a bit long and
unrealistic to me, particularly when it is about supporting undocumented
features.

I'm not suggesting to remove the conversion right now. It could be in
Org 10.0 or even Org 11.0 (As of Org 9.0.4 we still support variables
and functions deprecated since Org 8.0). But at least, at some point we
know that some compatibility layer can be removed instead of letting it
pile up indefinitely.

As a user, I expect changes to happens any time I update a software
I use to some higher major version. What I suggest is simply to send
a warning whenever a template using old methods is used, in addition to
doing the conversion. After a couple of years of seeing this message,
I'm pretty sure almost all users will have switched to the new pattern.

Anyway, I'm not going to oppose this patch just for that point. At least
you have my opinion.

> OK, no objections to a different implementation.  I am not familiar with
> pcase, looks general and useful, should learn about it.

Pattern matching is a very interesting feature, indeed. There are other
possible implementations of the same functin that do not rely on
`pcase', however. My point is more about reducing our use of `setq' at
a minimum, particularly since we moved to lexical scoping.

> Hmm, maybe I misunderstand pcase.  I was under the impression that when
> pcase does the match, it will bind the path to outline-path locally (with
> let or something similar), so that I can, in the scope of the current
> match, use setq to change the variable.
>
> Is my understanding incorrect here?

Your understanding is correct, and it is syntactically right to use
`setq' here.

However, as I pointed out above, it is better to use a let-binding over
a `setq' whenever possible since:

  1. it is faster,
  2. it makes the scope of the variable explicit.

> Well, I agree with you that we should not even have this code in here.  It
> is a hack to solve an issue I was not able to crack - maybe you can.  Let
> me explain:
>
> When I use customize to insert a template definition with the new
> file+olp+datetree target, I want the outline path to be optional.
>
> Here is the relevant part of the customize type definition:
>
>  (list :tag "File [ & Outline path ] & Date tree"
> (const :format "" file+olp+datetree)
> ,file-variants
> (repeat :tag "Outline path" :inline t
> (string :tag "Headline")))
>
> The problem is that customize sets this up assuming at least one string, in
> the customize buffer it looks like this
>
>             Target location: Value Menu File [ & Outline path ] & Date tree:
>             Filename       : Value Menu Literal:
>             Outline path:
>             INS DEL Headline:
>             INS
>             Template       : Value Menu String:
>
> As you can see, without the user clicking INS, there is already a string
> there. So the user would have to click DEL to make the old empty.  I
> figured people would forget all the time, therefore the hack to remove
> empty headlines.  It is critical that the outline path to be empty, because
> that is used to decide if the date trree will be on top level or under a
> heading.
>
> Do you or anyone know how to tweak customize to start out with an empty OLP
> for this application?  The we can remove that part entirely.  Otherwise, I
> am happy to make it a oneliner.

Wouldn't something like

         (choice
          (const :tag "Top level" :inline t nil)
          (repeat :tag "Outline path" :inline t
                  (string :tag "Headline")))

do the job?

> I'll take a look if it does make sense and do it if it is easy.  I see it
> as a separate issue since the week tree was implemented using a copy of the
> date tree function.  But I can merge this change into the patch I am making.

Of course, this is not a blocker. This came to mind when I was reading
your patch.

>> Ideally, a bunch of tests in test-org-capture.el would be nice.
>>
>
> Will do so after we have converged.

Great. Thank you.

Regards,

-- 
Nicolas Goaziou