Added support for target date variables in daily note template #781

riccardoferretti · 2021-10-09T16:02:28Z

fixes #776

We are adding new variables to the Foam daily note template.
These variables reflect the date of the note being created, so that a custom template can use them.

pderaaij

Looks good to me. Only the linter is not entirely happy at all places.

movermeyer

I have concerns about this approach, especially the naming, but also the premise that copying VSCode's snippet variables is a good idea at all.

I'd encourage you to write the end-user documentation (here and here) as part of this PR. This will help clarify your intentions to others and yourself.

movermeyer · 2021-10-12T12:02:43Z

packages/foam-vscode/src/features/create-from-template.ts

+ 'DAILY_NOTE_YEAR',
+ 'DAILY_NOTE_YEAR_SHORT',
+ 'DAILY_NOTE_MONTH',
+ 'DAILY_NOTE_MONTH_NAME',
+ 'DAILY_NOTE_MONTH_NAME_SHORT',
+ 'DAILY_NOTE_DATE',
+ 'DAILY_NOTE_DAY_NAME',
+ 'DAILY_NOTE_DAY_NAME_SHORT',
+ 'DAILY_NOTE_HOUR',
+ 'DAILY_NOTE_MINUTE',
+ 'DAILY_NOTE_SECOND',
+ 'DAILY_NOTE_SECONDS_UNIX',


My intention originally was to have all the Foam-specific variables namespaced with the FOAM_ prefix.

To keep it clear that this is a Foam variable, and not a VSCode variable.

DAILY_NOTE is very specific. The name gets awkward when we were to start using these in other places

(i.e., I think that any snippet variables should be ~~valid~~ useful in all scenarios they can be used in order to justify their inclusion. We shouldn't be adding any special logic/variables for daily notes; they are just notes like any other.)

Perhaps FOAM_DATE_*?

Thanks for the thorough answer @movermeyer.

Happy to go with FOAM_*, that's a good point.

More complicated is the topic of FOAM_DAILY_NOTE_* vs FOAM_DATE_*, I wonder if you are thinking of those variables as replacing the "date" snippet variables for our other purposes. The description of the PR is not super detailed, I will update it just to be sure.

These variables kinda make sense only in the context of a daily note (it is the date a daily note is referring to) so I think it's ok for these variables being empty in other templates

I think that any snippet variables should be valid in all scenarios they can be used

They would be valid, just empty - the same way FOAM_SELECTED_TEXT is valid and empty if there is no selected text or if we trigger the "New note" command.
Or maybe you are suggesting that in those cases they'd map to the current date? I actually wonder whether that would cause more confusion than just keeping them empty.

Before I address anything else I would like to be sure we are on the same page so far

They would be valid, just empty

I should have used the word "useful", not "valid". Discussed in the "Simplicity" section of my new comment

movermeyer · 2021-10-12T12:16:47Z

packages/foam-vscode/src/dated-notes.ts

+ dateVariables.set('DAILY_NOTE_HOUR', '00'); // The current hour in 24-hour clock format
+ dateVariables.set('DAILY_NOTE_MINUTE', '00'); // The current minute
+ dateVariables.set('DAILY_NOTE_SECOND', '00'); // The current second


These are always returning 00?

I think the value probably makes sense if you need to have completeness relative to the VSCode snippets (since you only have a date, and it should be the start of the date).
But in what scenarios would you expect a user to use/want these?

In general, is there enough value to justify having a complete set of variables just to match the VSCode variables (which we already know aren't great for user's flexibility)? Every variable is additional tech debt.

Perhaps a better option instead of this PR is introduce a single variable $FOAM_DATE_FORMAT to solve this issue and this issue.

I would agree we should just kill the hour/minute/second, they don't really add anything except the "mirroring" but even then it's kinda empty.

I feel the $FOAM_DATE_FORMAT solves a different issue? this comment is part of the reason why I am not sure we are thinking about the same problem, am I missing something?

I feel the $FOAM_DATE_FORMAT solves a different issue?

Yeah, there are two discussions mixing here. The relationship to is discussed in the "Expressivity" section of my new comment.

movermeyer · 2021-10-12T12:29:52Z

packages/foam-vscode/src/test/test-utils-vscode.ts

+export const readFile = async (filepath?: string[] | URI): Promise<string> => {
+ const rootUri = vscode.workspace.workspaceFolders[0].uri;
+ filepath = filepath ?? [randomString() + '.md'];


I think I'm missing something...

Where is this used?
Why is filepath optional?
Why is there randomness in the filepath assignment?

oh my, this is my bad copying of the writeFile function. I wanted them to be mirror of each other, but obviously the optionality of the argument shouldn't fall into that 🤦‍♂️

This is actually not used anywhere because in the end I just ended up using the editor itself to check the generation of the note, so we can actually get rid of it

movermeyer · 2021-10-13T12:44:18Z

@riccardoferretti I think we're actually very similar in our thinking, but I didn't put enough time into writing it all out. 😅

I think there are two separate topics to discuss:

Simplicity (i.e. Do we need daily-note-specific variables?)
Expressivity (i.e. Should we copy VSCode's date variables interface?)

Simplicity

My gut instinct when I saw the issue was to create a Foam-specific variable that could replace the use of CURRENT_* in templates.
i.e., the new variable would be equivalent to CURRENT_*, but always populated according to the appropriate frame of reference.

In this PR, the daily note variables are FOAM_DATE_* = <target_date> || null, so they are only useful in the context of daily notes
What I had in mind is: FOAM_DATE_* = <target_date> ?? <current_date>. It could be useful in any context (daily note templates, but also regular ones)

Benefits

Your daily notes don't have any magic in them: Daily and non-daily notes look and behave the exact same way
No context-specific variables: IMO snippet variables should be useful in all contexts (within reason) in order to justify their existence
- They are an API that we have to support forever. The fewer the better; IMO we should take the time to ensure we can't the interface simpler.

Detriments

"Perhaps it'll be too complicated to explain the frame of reference?"
- I don't think we have to explain a new concept / I don't think users will ever be surprised. I think we can state in the documentation "FOAM_DATE_* variables are how you insert dates in templates" and it'll just work the way they expect. We might explain the difference between FOAM_DATE_* and VSCode's CURRENT_* in a "pro-tip" box or whatever, but the basic documentation is very simple

Expressivity

Relation to #564

Since this involves customizing date formats in daily notes, and you will also want to use the new variables in filepaths, it is very adjacent to this issue. I think they can be solved at the same time? We were hoping for a solution more flexible than what VSCode snippet variables can offer us (and there are similar issues in the upstream complaining about the limitations of the existing date variables), so I'm not convinced that copying their variables is a good idea. As discussed briefly on that issue, something similar to $FOAM_DATE_FORMAT (bad name, I know) might be a good candidate.

Example daily-note.md template:

---
foam_template:
  filepath: 'journal/${FOAM_DATE_FORMAT}.md'
---
# ${FOAM_DATE_FORMAT}

## What did I do today?

Question: What if a user actually wants the current date in their "relative daily note"?

(The current PR also works in this way. I just wanted to comment here since I had asked myself this question.)

They might want a "created on" in their notes.

Desired note contents:

# 2021-12-13 <- Some date in the future

# $FOAM_TITLE

Created on 2021-10-13 <- Today's date

Answer: In this case, they can make use of the existing CURRENT_* variables.

# $FOAM_DATE_FORMAT <- Some date in the future

# $FOAM_TITLE

Created on $CURRENT_YEAR-$CURRENT_MONTH-$CURRENT_DATE <- Today's date

The current PR also works in this way. I just wanted to comment here since I had asked myself this question.

riccardoferretti · 2021-10-17T20:50:12Z

I agree with everything you wrote, happy to go in that direction, except on the topic of FOAM_DATE_FORMAT, which feels like an add-on to this conversation (the value of which I am still questioning), so I would bracket it out from this PR

riccardoferretti · 2021-10-25T17:39:14Z

@movermeyer it took a bit longer because in the end I decided to move the resolution to the resolver itself.. can you take a look and see if it fits with the original design of the resolver?

movermeyer

@riccardoferretti I still don't understand your changes to packages/foam-vscode/src/test/test-utils-vscode.ts, and I noticed a few oddities, but things look much better now. 👏

packages/foam-vscode/src/features/create-from-template-variables.spec.ts

Discussion had. Changes implemented.

riccardoferretti · 2021-10-26T11:00:17Z

I have now implemented the feedback, and ready to merge.
I have also cleaned the useless fn in test-utils-vscode.ts.
I am curious about the oddities, feel free to share them or not.
Will merge in the eve.

movermeyer

@riccardoferretti LGTM.

I am curious about the oddities, feel free to share them or not.

The oddities were just the bits I pointed out in the last review, which you have now fixed. 👍
(I could have been clearer 🙃)

I've added two commits to this PR to document the feature, and add a CHANGELOG entry. Take a look.

riccardoferretti · 2021-10-27T08:50:32Z

Yup thanks for the docs!

riccardoferretti requested review from pderaaij and movermeyer October 9, 2021 16:02

pderaaij approved these changes Oct 11, 2021

View reviewed changes

movermeyer previously requested changes Oct 12, 2021

View reviewed changes

riccardoferretti added 3 commits October 23, 2021 16:34

added support for target date variables in daily note template

80a27c2

renamed DAILY_NOTE_* to FOAM_DATE_*

0260b66

added FOAM_DATE_* variables to resolver

fbde5d0

riccardoferretti force-pushed the features/daily-notes-variables branch from 2e22d14 to fbde5d0 Compare October 25, 2021 17:17

movermeyer reviewed Oct 25, 2021

View reviewed changes

riccardoferretti added 4 commits October 26, 2021 10:32

removed unused test function

e9353ae

fixed padding and improved tests

65de5bb

added 'FOAM_DATE_SECONDS_UNIX' to tests

37a3531

make lint happy again

f9c2b63

movermeyer added 2 commits October 26, 2021 09:23

Document FOAM_DATE_* template variables

f5ac448

Add CHANGELOG entry

745ef93

movermeyer approved these changes Oct 26, 2021

View reviewed changes

riccardoferretti merged commit d31e094 into master Oct 27, 2021

riccardoferretti deleted the features/daily-notes-variables branch October 27, 2021 08:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for target date variables in daily note template #781

Added support for target date variables in daily note template #781

riccardoferretti commented Oct 9, 2021

pderaaij left a comment

movermeyer left a comment

movermeyer Oct 12, 2021 •

edited

Loading

riccardoferretti Oct 13, 2021

movermeyer Oct 13, 2021

movermeyer Oct 12, 2021

riccardoferretti Oct 13, 2021

movermeyer Oct 13, 2021

movermeyer Oct 12, 2021

riccardoferretti Oct 12, 2021

movermeyer commented Oct 13, 2021 •

edited

Loading

riccardoferretti commented Oct 17, 2021

riccardoferretti commented Oct 25, 2021

movermeyer left a comment •

edited

Loading

riccardoferretti commented Oct 26, 2021

movermeyer left a comment

riccardoferretti commented Oct 27, 2021

Added support for target date variables in daily note template #781

Added support for target date variables in daily note template #781

Conversation

riccardoferretti commented Oct 9, 2021

pderaaij left a comment

Choose a reason for hiding this comment

movermeyer left a comment

Choose a reason for hiding this comment

movermeyer Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

riccardoferretti Oct 13, 2021

Choose a reason for hiding this comment

movermeyer Oct 13, 2021

Choose a reason for hiding this comment

movermeyer Oct 12, 2021

Choose a reason for hiding this comment

riccardoferretti Oct 13, 2021

Choose a reason for hiding this comment

movermeyer Oct 13, 2021

Choose a reason for hiding this comment

movermeyer Oct 12, 2021

Choose a reason for hiding this comment

riccardoferretti Oct 12, 2021

Choose a reason for hiding this comment

movermeyer commented Oct 13, 2021 • edited Loading

Simplicity

Expressivity

Relation to #564

Question: What if a user actually wants the current date in their "relative daily note"?

riccardoferretti commented Oct 17, 2021

riccardoferretti commented Oct 25, 2021

movermeyer left a comment • edited Loading

Choose a reason for hiding this comment

riccardoferretti commented Oct 26, 2021

movermeyer left a comment

Choose a reason for hiding this comment

riccardoferretti commented Oct 27, 2021

movermeyer Oct 12, 2021 •

edited

Loading

movermeyer commented Oct 13, 2021 •

edited

Loading

movermeyer left a comment •

edited

Loading