Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add remaining front matter retrieval functions #468

Merged
merged 2 commits into from
Oct 26, 2024

Conversation

jeanphilippegg
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Owner

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 protesilaos merged commit 3517ba2 into protesilaos:main Oct 26, 2024
@protesilaos
Copy link
Owner

Merged, thank you!

protesilaos added a commit that referenced this pull request Oct 26, 2024
This was done in issue 467: <#467>.

The commit is by Jean-Philippe Gagné Guay, which was done in pull
request 468: <#468>.
@jeanphilippegg
Copy link
Contributor Author

jeanphilippegg commented Oct 26, 2024

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).

@jeanphilippegg jeanphilippegg deleted the retrieval-functions branch October 26, 2024 14:20
@protesilaos
Copy link
Owner

protesilaos commented Oct 27, 2024 via email

@jeanphilippegg
Copy link
Contributor Author

Perfect!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants