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

Fixed small bug in note.from_lines #67

Merged
merged 4 commits into from
Jan 3, 2023
Merged

Fixed small bug in note.from_lines #67

merged 4 commits into from
Jan 3, 2023

Conversation

s-cassidy
Copy link
Contributor

If a note had frontmatter consisting of exactly one empty field, yaml.loads would return that field as a string rather than a table, causing an exception in note.from_lines.

Copy link
Owner

@epwalsh epwalsh left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @s-cassidy! Just one minor suggestion

@@ -209,6 +209,9 @@ note.from_lines = function(lines, path, root)
if #frontmatter_lines > 0 then
local frontmatter = table.concat(frontmatter_lines, "\n")
local ok, data = pcall(yaml.loads, frontmatter)
if type(data) == 'string' then
data = {data = nil}
Copy link
Owner

Choose a reason for hiding this comment

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

How about just this:

Suggested change
data = {data = nil}
data = {}

@s-cassidy
Copy link
Contributor Author

s-cassidy commented Jan 3, 2023

That's my poor Lua knowledge showing me up as I didn't realize nil table entries effectively don't exist. I've made the change if you want to just go with it. However, this is closer to what I was thinking

if type(data) == 'string' then
  local field = data
  field = field:gsub("^%s*(.-):?%s*$", "%1") 
  data = {}
  data[field] = {}
end

This way the following code still gets the name of the empty field (with trailing whitespace and : removed) with an empty table as the value.

Copy link
Owner

@epwalsh epwalsh left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@epwalsh epwalsh merged commit 0739833 into epwalsh:main Jan 3, 2023
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