-
Notifications
You must be signed in to change notification settings - Fork 668
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
Allow for YAML metadata in templates #655
Allow for YAML metadata in templates #655
Conversation
305ae48
to
f990b4a
Compare
The general approach looks good to me, a few comments:
|
daf2a94
to
0b5bbec
Compare
@riccardoferretti, I think this is ready for review. Since you last looked, I've implemented the IMO, it looks pretty slick: Replying to your previous comment.
💯, that's the next step after this PR.
👍
It wasn't very complicated to support both (with the caveat that users cannot use the full flexibility of the YAML syntax if they want to have the metadata within an existing Frontmatter bock) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the screenshots look pretty cool, thanks for this!
I have added a few comments, mostly minor, let me know your thoughts
@@ -218,15 +299,41 @@ function currentDirectoryFilepath(filename: string) { | |||
return Uri.joinPath(currentDir, filename); | |||
} | |||
|
|||
export function determineDefaultFilepath( | |||
resolvedValues: Map<string, string>, | |||
templateMetadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this an implicit any
? in that case I am surprised it didn't raise a warning from the linter.
I also noticed in other places you are using object
for metadata.. given that we have a specific shape for the template metadata, could it be helpful to map it onto a type/interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am surprised it didn't show up as a warning in the PR, I thought the integration would do that (kinda like when there is an unused import/variable)... mm, to be looked into at some point.
Given the metadata has a specific type, I would just defined that, as it also provides a good in-code documentation for what info can be found (and is allowed) as template metadata, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the metadata has a specific type, I would just defined that, as it also provides a good in-code documentation for what info can be found (and is allowed) as template metadata, wdyt?
@riccardoferretti Sounds good to me. I won't have time to make this change until next week. If you're eager, we can ship this now, and I'll do that refactor as part of my next PR.
0b5bbec
to
e2e9e27
Compare
dadc700
to
7dac126
Compare
7dac126
to
9731c80
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
happy to merge this in the meantime, will do a release later so the feature will already be available. thanks!
Just came across an issue that I couldn't find in the docs, started reading the code and found |
What are you trying to accomplish?
As part of #563, we are adding a mechanism to provide metadata within a Frontmatter block within a Foam note template.
What approach did you choose and why?
As noted in this comment, it's nicer to be able to add the metadata to an existing YAML frontmatter block.
However, it is also very difficult to programmatically remove an entry from a YAML mapping without disrupting the rest of the YAML.
So in this PR, I allow for both. If you have an existing YAML frontmatter block, and you're willing to abide by fairly strict formatting, then you can add the metadata to the existing frontmatter block.
If for some reason, you cannot use the restricted syntax, you are still able to provide a second, Foam-specific YAML frontmatter block at the start of the template instead.
What should reviewers focus on?
Not quite there
This PR does not succeed at fully implementing the desired behaviour.
I had dreamed of being able to use any VS Code snippet variable in the
filepath
.This would allow
Foam: Open Daily Note
to make use of templates easily.However, this is not currently possible. VS Code does not currently offer an interface for resolving the snippet variables without opening the snippet in an editor for the user.
In the future, we might vendor the relevant bits snippet resolution code, or submit a patch to VS Code to expose their variable resolution functionality as part of the API.
The PR as it stands still offers value, since it allows you to define relative or absolute directories to store the files into, instead of the default
Foam: Create New Note
behaviour of opening the file in the same directory as the active file.Example of using metadata to specify a directory relative to the current workspace
Next steps
Foam: Open Daily Note
use templates