~protesilaos/denote

7 2

[PATCH] Allow users to change front matter by setq

Details
Message ID
<86k05gsqsg.fsf@nobiot.com>
DKIM signature
missing
Download raw message
Hi Prot,

Denote is a great package.  Thank you for taking the lead in organizing
the contributors and bringing it to the world.  It's a beautiful piece
of software -- focused feature set and great documentation.

Here is a patch that I would appreciate if you consider.  The user
manual talks about ways to personalize the front matter.  It currently
poses a challenge because the way how
'denote-org/yaml/toml/text-front-matter' and 'denote-file-types' are
defined.  You will need to change both of them once 'denote' has been
loaded or compiled.

Explaining this is a little harder than showing the code.  Please have a
look at the attached patch with my commit message.  I have tested the
patch with the Org file type.

In the manual you also noted that you wished it to be only for advanced
configuration, so this challenge might have been intentional.  In this
case, you can disregard the patch, of course.  Nevertheless, this open
message with a patch might help others when they struggle with
personalizing the front matter in Denote.

Thank you
nobiot

---

P.S. My first time using SourceHut mailing list to send a patch and
first time to generate a patch.  I hope this has worked and does not
cause any issue on your end -- I am not even sure if the attachment has
arrived as intended.  Please advise if you have issues with this email
and patch file.
Details
Message ID
<875yh0oho1.fsf@protesilaos.com>
In-Reply-To
<86k05gsqsg.fsf@nobiot.com> (view parent)
DKIM signature
missing
Download raw message
> From: Noboru Ota <me@nobiot.com>
> Date: Mon,  3 Oct 2022 20:30:55 +0200
>
>>From f007324a3a28a7fffc83b4af5a3b2daecb3c5182 Mon Sep 17 00:00:00 2001
> From: Noboru Ota <me@nobiot.com>
> Date: Mon, 3 Oct 2022 20:11:13 +0200
> Subject: [PATCH] Allow users to change front matter by setq
>
> Change how 'denote-org/yaml/toml/text-front-matter' and
> 'denote-file-types' are defined.

> [... 74 lines elided]

Thank you, nobiot!

I am just sending this now to inform you that I got your message and it
is fine.  The patch is okay in terms of formatting.  I will review and
install it tomorrow morning, as I am about to go to bed now.

All the best,
Prot

-- 
Protesilaos Stavrou
https://protesilaos.com
Details
Message ID
<871qronzv6.fsf@protesilaos.com>
In-Reply-To
<875yh0oho1.fsf@protesilaos.com> (view parent)
DKIM signature
missing
Download raw message
> From: Protesilaos Stavrou <info@protesilaos.com>
> Date: Mon,  3 Oct 2022 22:01:34 +0300
>
>> From: Noboru Ota <me@nobiot.com>
>> Date: Mon,  3 Oct 2022 20:30:55 +0200
>>
>>>From f007324a3a28a7fffc83b4af5a3b2daecb3c5182 Mon Sep 17 00:00:00 2001
>> From: Noboru Ota <me@nobiot.com>
>> Date: Mon, 3 Oct 2022 20:11:13 +0200
>> Subject: [PATCH] Allow users to change front matter by setq
>>
>> Change how 'denote-org/yaml/toml/text-front-matter' and
>> 'denote-file-types' are defined.
>
>> [... 74 lines elided]
>
> Thank you, nobiot!
>
> I am just sending this now to inform you that I got your message and it
> is fine.  The patch is okay in terms of formatting.  I will review and
> install it tomorrow morning, as I am about to go to bed now.

Hello again and good day from my side!

I am now reviewing the patch.  The change to 'denote-file-types' is
fine, though the use of 'eval' in 'denote--front-matter' concerns me.  I
am conditioned to think that 'eval' is generally too powerful and should
be used with caution.  Do you think it is necessary here?  Perhaps
something like the following would be a good alternative?

    (defun denote--front-matter (file-type)
      "Return front matter based on FILE-TYPE."
      (let ((prop (plist-get
                   (alist-get file-type denote-file-types)
                   :front-matter)))
        (if (symbolp prop)
            (symbol-value prop)
          prop)))

All the best,
Prot

P.S. We want to turn 'denote-file-types' into a user option at some
point and standardise on a specific format for its plist.  This has not
happened yet because the variable has not enjoyed extensive usage.

-- 
Protesilaos Stavrou
https://protesilaos.com
Details
Message ID
<861qroozwq.fsf@nobiot.com>
In-Reply-To
<875yh0oho1.fsf@protesilaos.com> (view parent)
DKIM signature
missing
Download raw message
Sorry my previous reply didn't seem to go back to the mailling list.  I
am resending the same message, this time hopefully to the mailing list.
Apologies.

   > Hello again and good day from my side!
   > I am now reviewing the patch.  The change to 'denote-file-types' is
   [ 10 more citation lines. Click/Enter to show. ]
   > fine, though the use of 'eval' in 'denote--front-matter' concerns me.  I
   > am conditioned to think that 'eval' is generally too powerful and should
   > be used with caution.  Do you think it is necessary here?  Perhaps
   > something like the following would be a good alternative?
   >
   >     (defun denote--front-matter (file-type)
   >       "Return front matter based on FILE-TYPE."
   >       (let ((prop (plist-get
   >                    (alist-get file-type denote-file-types)
   >                    :front-matter)))
   >         (if (symbolp prop)
   >             (symbol-value prop)
   >           prop)))

Good morning, Prot.

I have tested your suggestion on my end and it works.  The use of
'symbol-value' more precisely expresses the intent of my suggested
change than 'eval'.  I do not have any information or experience to
comment on your concern about 'eval' but I suppose 'symbol-value' is
narrower in scope so generally safer.

I am assuming that you will apply the new version of the patch on your
end without needing me sending another.  I will be happy to, if you
would like me to.  Please advise.

In addition, it would be a new to-do item outside the scope of this
patch, but I noticed that you could set the front-matter to nil, and
this case is not treated gracefully. You get a message saying "let:
Wrong type argument: stringp, nil" and the new note gets created with no
front-matter (as intended).  It should be considered an edge case but
perhaps it's something you would like to deal with when you are turning
'denote-file-types' into a user option.

Have a great day. Cheers.

nobiot.
Details
Message ID
<87tu4kkm7d.fsf@protesilaos.com>
In-Reply-To
<861qroozwq.fsf@nobiot.com> (view parent)
DKIM signature
missing
Download raw message
> From: Noboru Ota <me@nobiot.com>
> Date: Tue,  4 Oct 2022 08:39:49 +0200
>
> Sorry my previous reply didn't seem to go back to the mailling list.  I
> am resending the same message, this time hopefully to the mailing list.
> Apologies.
>
>    > Hello again and good day from my side!
>    > I am now reviewing the patch.  The change to 'denote-file-types' is
>    [ 10 more citation lines. Click/Enter to show. ]
>    > fine, though the use of 'eval' in 'denote--front-matter' concerns me.  I
>    > am conditioned to think that 'eval' is generally too powerful and should
>    > be used with caution.  Do you think it is necessary here?  Perhaps
>    > something like the following would be a good alternative?
>    >
>    >     (defun denote--front-matter (file-type)
>    >       "Return front matter based on FILE-TYPE."
>    >       (let ((prop (plist-get
>    >                    (alist-get file-type denote-file-types)
>    >                    :front-matter)))
>    >         (if (symbolp prop)
>    >             (symbol-value prop)
>    >           prop)))
>
> Good morning, Prot.

Hello again!

> I have tested your suggestion on my end and it works.  The use of
> 'symbol-value' more precisely expresses the intent of my suggested
> change than 'eval'.  I do not have any information or experience to
> comment on your concern about 'eval' but I suppose 'symbol-value' is
> narrower in scope so generally safer.

Very well, let's go with this.

> I am assuming that you will apply the new version of the patch on your
> end without needing me sending another.  I will be happy to, if you
> would like me to.  Please advise.

Yes, sure.  I want to install it on your behalf, since it is your change
basically and it is appropriate to be credited for it.  I will update
the manual's "Acknowledgements" as well.  Is it okay if I do this?

[ For the interest of the mailing list: all major contributions require
  copyright assignment to the Free Software Foundation.  The author here
  has done this already (e.g. check the 'org-transclusion' package on
  GNU ELPA). ]

> In addition, it would be a new to-do item outside the scope of this
> patch, but I noticed that you could set the front-matter to nil, and
> this case is not treated gracefully. You get a message saying "let:
> Wrong type argument: stringp, nil" and the new note gets created with no
> front-matter (as intended).  It should be considered an edge case but
> perhaps it's something you would like to deal with when you are turning
> 'denote-file-types' into a user option.

Agreed.  We should handle this internally so that a nil value is read as
an empty string.  It can be done now without accounting for the
eventually of a user option.

> Have a great day. Cheers.

You too!  Thank you!

-- 
Protesilaos Stavrou
https://protesilaos.com
Details
Message ID
<868rlvq3d3.fsf@nobiot.com>
In-Reply-To
<875yh0oho1.fsf@protesilaos.com> (view parent)
DKIM signature
missing
Download raw message
Protesilaos Stavrou <info@protesilaos.com> writes:

> Yes, sure.  I want to install it on your behalf, since it is your change
> basically and it is appropriate to be credited for it.  I will update
> the manual's "Acknowledgements" as well.  Is it okay if I do this?

Sure, no problem.  Thank you.  If you need a name, perhaps like this
with my full name (?):

    Noboru Ota (nobiot)

> [ For the interest of the mailing list: all major contributions require
>   copyright assignment to the Free Software Foundation.  The author here
>   has done this already (e.g. check the 'org-transclusion' package on
>   GNU ELPA). ]

Yes, I have assigned copyright to FSF and both 'org-transclusion' and
'org-remark' that I have authored are on GNU ELPA.

Thanks again.

nobiot
Details
Message ID
<87a66blu0j.fsf@protesilaos.com>
In-Reply-To
<868rlvq3d3.fsf@nobiot.com> (view parent)
DKIM signature
missing
Download raw message
> From: Noboru Ota <me@nobiot.com>
> Date: Tue,  4 Oct 2022 12:39:52 +0200
>
> Protesilaos Stavrou <info@protesilaos.com> writes:
>
>> Yes, sure.  I want to install it on your behalf, since it is your change
>> basically and it is appropriate to be credited for it.  I will update
>> the manual's "Acknowledgements" as well.  Is it okay if I do this?
>
> Sure, no problem.  Thank you.  If you need a name, perhaps like this
> with my full name (?):
>
>     Noboru Ota (nobiot)

Very well!  I did it in commit 9afdf86.  I also added your name to the
manual's "Acknowledgements" section.

I made a mistake in spelling your name "Noburu" instead of "Noboru".  I
pushed a commit to fix the typo in the manual, though the error persists
in the commit logs.  Sorry!

>> [ For the interest of the mailing list: all major contributions require
>>   copyright assignment to the Free Software Foundation.  The author here
>>   has done this already (e.g. check the 'org-transclusion' package on
>>   GNU ELPA). ]
>
> Yes, I have assigned copyright to FSF and both 'org-transclusion' and
> 'org-remark' that I have authored are on GNU ELPA.

I know.  It was for the interest of the list.

I have a TODO about 'org-transclusion' and the 'denote:' link type, but
I have not found the time to check the specifics and contact you about
it.  Will happen eventually.

-- 
Protesilaos Stavrou
https://protesilaos.com
Details
Message ID
<87a66blkzn.fsf@protesilaos.com>
In-Reply-To
<861qroozwq.fsf@nobiot.com> (view parent)
DKIM signature
missing
Download raw message
> From: Noboru Ota <me@nobiot.com>
> Date: Tue,  4 Oct 2022 08:39:49 +0200

> [... 34 lines elided]

> In addition, it would be a new to-do item outside the scope of this
> patch, but I noticed that you could set the front-matter to nil, and
> this case is not treated gracefully. You get a message saying "let:
> Wrong type argument: stringp, nil" and the new note gets created with no
> front-matter (as intended).  It should be considered an edge case but
> perhaps it's something you would like to deal with when you are turning
> 'denote-file-types' into a user option.

Hello again, nobiot!

About the above quote, I implemented the requisite change to a private
function.

    commit 3775a61028c4e12530392018428b95279aabcabf
    Author: Protesilaos Stavrou <info@protesilaos.com>
    Date:   Tue Oct 4 17:24:45 2022 +0300

        Allow nil :front-matter in denote-file-types

        The 'denote-file-types' is, for the time being, specified as a
        'defvar' for advanced users.  We do eventually want to turn it into a
        defcustom, assuming there is a need for it (we will figure it out
        through further testing).

        This change allows the ':front-matter' property to accept a nil value,
        which we internally read as an empty string.

        Thanks to Noboru Ota (nobiot) for pointing this out on the mailing list:
        <https://lists.sr.ht/~protesilaos/denote/%3C86k05gsqsg.fsf%40nobiot.com%3E>.

     denote.el | 7 ++++---
     1 file changed, 4 insertions(+), 3 deletions(-)

All the best and apologies once again for mispelling your name earlier,
Prot

-- 
Protesilaos Stavrou
https://protesilaos.com
Reply to thread Export thread (mbox)