protesilaos / denote

Simple notes for Emacs with an efficient file-naming scheme
https://protesilaos.com/emacs/denote
GNU General Public License v3.0
541 stars 55 forks source link

Add remaining front matter retrieval functions #468

Closed jeanphilippegg closed 1 month ago

jeanphilippegg commented 1 month ago

In response to #467 and to allow us to handle the front matter in future development, I have added the missing front matter retrieval functions in denote-file-types. I assumed the signature would be included eventually.

Since we still do not use them, this should not break anything. I created and renamed a note of each type to be sure.

Note that I renamed :date-format-function to :date-value-function to be consistent with the other components.

jeanphilippegg commented 1 month ago

I have added a commit to simplify denote-format-string-for-{org/md}-front-matter. We do not need to check that the parameter is a string. I checked all code paths and it is not possible for them to be called with a nil value. That would be a mistake. If it were possible before, it may have been fixed as part of the refactoring of the renaming commands.

Now, the only remaining possibility is if the user calls denote-rename-file or denote-add-front-matter from lisp with a nil title, but that would be a mistake. The docstring says that it is a string parameter. Interactively, all is fine.

This is ready to be merged!

protesilaos commented 1 month ago

from lisp with a nil title, but that would be a mistake

Do you think we should leave this as-is or interpret nil as an empty string in this case? I am fine with what we have. Just asking.

protesilaos commented 1 month ago

Merged, thank you!

jeanphilippegg commented 1 month ago

Do you think we should leave this as-is or interpret nil as an empty string in this case?

I think a nil value for a parameter makes sense when it means "not set", such as with optional parameters. In the case of these commands, the parameter is mandatory. It seems reasonable to me to expect users to call them with arguments of the appropriate type.


Since we are at it, in commit 55e923f, about denote-format-file-name, you made this comment in git:

We do not want to limit this to empty strings, because there are cases we will need to arrange for the return value to be that. Whereas we get nil for free.

In this case, I can understand that the title and signature can have this "not set" meaning. My preference is still to enforce a string because the parameters are mandatory. But I am conflicted, because at some point, I may suggest to make the id optional and nil may be better than an empty string to mean an absent identifier. It would not really be an issue, but it may be confusing that nil is permitted for a paramater, but not the others.

You can remove it if you want. It would be safe: we never call it with nil in the code (I just checked.).


Also, I am okay if you would decide to revert my last commit (about denote-format-string-for-{org/md}-front-matter).

protesilaos commented 4 weeks ago

From: Jean-Philippe Gagné Guay @.***> Date: Sat, 26 Oct 2024 06:40:10 -0700

I think a nil value for a parameter makes sense when it means "not set", such as with optional parameters. In the case of these commands, the parameter is mandatory. It seems reasonable to me to expect users to call them with arguments of the appropriate type.

This is fine, thank you!

Since we are at it, in commit 55e923f, about denote-format-file-name, you made this comment in git:

We do not want to limit this to empty strings, because there are cases we will need to arrange for the return value to be that. Whereas we get nil for free.

In this case, I can understand that the title and signature can have this "not set" meaning. My preference is still to enforce a string because the parameters are mandatory. But I am conflicted, because at some point, I may suggest to make the id optional and nil may be better than an empty string to mean an absent identifier. It would not really be an issue, but it may be confusing that nil is permitted for a paramater, but not the others.

Indeed, it is better to have this nuanced treatment of the value.

You can remove it if you want. It would be safe: we never call it with nil in the code (I just checked.).


Also, I am okay if you would decide to revert my last commit (about denote-format-string-for-{org/md}-front-matter).

No, let's keep it as-is. I just want to ask to be sure I am on the same page as you.

-- Protesilaos Stavrou https://protesilaos.com

jeanphilippegg commented 4 weeks ago

Perfect!