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

Parse frontmatter and use frontmatter.title as heading (fixes #174) #191

Merged
merged 8 commits into from Aug 3, 2020

Conversation

anku255
Copy link
Contributor

@anku255 anku255 commented Jul 29, 2020

Summary

fixes #174

This PR does the following things -

  • Add frontmatter metadata to the NoteGraph node.
  • Parse heading from the frontmatter and give it preference over the # or = heading.
  • If the frontmatter exists but the title (heading) doesn't then generate the heading below the frontmatter.
  • Show a progress bar (window.withProgress) while the Janitor runs.

Test Plan

I have tested with the following types of markdown notes.

Input:

---
title: Note Title From Heading
---

Note content

Output: Doesn't generate the heading and uses frontmatter.title as note.title.

Input:

inbox.md

---
other: some other data
---

Note content

Output:

---
other: some other data
---

# Inbox

Note content

Input:

inbox.md

---
other: some other data
---
note content starts without a newline

Output:

---
other: some other data
---

# Inbox

note content starts without a newline

visit(tree, node => {
if (node.type === 'yaml') {
frontmatter = parseYAML(node.value as string);
Copy link
Contributor Author

@anku255 anku255 Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am using the yaml package for parsing the frontmatter. Is that okay?

Copy link
Collaborator

@jevakallio jevakallio Jul 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only downside is that it's 27Kb minzipped, and theoretically foam-core could be used in a browser environment, so it adds a bit of chonk to the bundle.

I don't really see a good alternative right now, since I don't want to implement a partial yaml subset parser

Copy link
Collaborator

@riccardoferretti riccardoferretti Jul 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I'd say it's probably ok for now, we might want to revisit for optimizations once as part of the browser release

@anku255 anku255 requested a review from jevakallio Jul 29, 2020
Copy link
Collaborator

@jevakallio jevakallio left a comment

@anku255 I was testing this and managed to break this when the frontmatter wasn't fully valid yaml:

foo: bar
baz

I'm not sure if related to invalid yaml, but after testing with an invalid header, the Janitor replaced the content of the file with a old, cached version of the document.

I didn't have time to test it in detail!

visit(tree, node => {
if (node.type === 'yaml') {
frontmatter = parseYAML(node.value as string);
Copy link
Collaborator

@riccardoferretti riccardoferretti Jul 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I'd say it's probably ok for now, we might want to revisit for optimizations once as part of the browser release

packages/foam-core/src/note-graph.ts Outdated Show resolved Hide resolved
packages/foam-vscode/src/features/janitor.ts Outdated Show resolved Hide resolved
packages/foam-vscode/src/features/janitor.ts Show resolved Hide resolved
@anku255
Copy link
Contributor Author

@anku255 anku255 commented Aug 2, 2020

@anku255 I was testing this and managed to break this when the frontmatter wasn't fully valid yaml:
...
...

@jevakallio I don't think the invalid yaml is the issue here. If the yaml is invalid then you should see this error -

Command 'Foam: Run Janitor (Experimental)' resulted in an error (Implicit map keys need to be followed by map values)

I think this problem is related to inconsistent NoteGraph. For some reason, the file changes are not reflected properly while running the Janitor.

The Janitor command works fine on a freshly loaded VS Code window but if you make changes to some files (be it dirty or nondirty), Janitor may produce unexpected results.

To reproduce this you can try the following steps: -

  1. Have a file with invalid frontmatter. For example
---
title: Note Title
foo
----
  1. Launch Extension Host (F5).

  2. Run Janitor. (It should show the invalid frontmatter error).

  3. Make the frontmatter valid and (save the file).

---
title: Note Title
---
  1. Run Janitor again. (It still shows the invalid frontmatter error).

Do you have any idea why this might be happening? My initial understanding is that the NoteGraph is not updated after every file change.

The createNoteFromMarkdown is only called on the first execution of Janitor Command but not on the subsequent runs. I am not sure if this is what's happening but my console.log debugging proves this.

Copy link
Collaborator

@riccardoferretti riccardoferretti left a comment

looking good to me, just left a couple of comments

packages/foam-core/test/core.test.ts Show resolved Hide resolved
@jevakallio jevakallio self-requested a review Aug 3, 2020
Copy link
Collaborator

@jevakallio jevakallio left a comment

Thanks @anku255 for the explanation. Seems like the root cause for this issue is already on master and needs to be addressed separately

@anku255
Copy link
Contributor Author

@anku255 anku255 commented Aug 3, 2020

Thanks @anku255 for the explanation. Seems like the root cause for this issue is already on master and needs to be addressed separately

Thank you for your approval. But don't merge it just yet. I will be doing the changes suggested by @riccardoferretti soon.

@riccardoferretti riccardoferretti merged commit bcab419 into master Aug 3, 2020
3 checks passed
@riccardoferretti riccardoferretti deleted the 174-add-frontmatter branch Aug 3, 2020
@riccardoferretti
Copy link
Collaborator

@riccardoferretti riccardoferretti commented Aug 3, 2020

Thanks @anku255 for the explanation. Seems like the root cause for this issue is already on master and needs to be addressed separately

Thank you for your approval. But don't merge it just yet. I will be doing the changes suggested by @riccardoferretti soon.

hey @anku255 we ended up merging this, but feel free to make the changes on another branch, or also directly on refactor/note-id if you think you'd run into conflicts, after all it's all minor things

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

Successfully merging this pull request may close these issues.

3 participants