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

feat: add option for disabling frontmatter updates #816

Closed
wants to merge 1 commit into from

Conversation

thetwistedlogic
Copy link

While I do enjoy Obsidian creating yaml frontmatter for me, I've found strange issues with the fronmatter updating functionality. I've added the option to disable it.

Copy link

@adamtajti adamtajti left a comment

Choose a reason for hiding this comment

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

I'm not a maintainer, but I skimmed through a couple of pull requests before planning to create one of my own and I left a couple of comments here that could be of use to improve this work.

@@ -276,7 +276,7 @@ Client.templates_dir = function(self, workspace)
return nil
end

--- Determines whether a note's frontmatter is managed by obsidian.nvim.
--- Determines whether a note's frontmatter is created by obsidian.nvim.

Choose a reason for hiding this comment

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

I think that this term should be kept as managed if both the update and the create functionality is going to be supported.

@@ -18,7 +18,8 @@ local config = {}
---@field follow_url_func fun(url: string)|?
---@field follow_img_func fun(img: string)|?
---@field note_frontmatter_func (fun(note: obsidian.Note): table)|?
---@field disable_frontmatter (fun(fname: string?): boolean)|boolean|?

Choose a reason for hiding this comment

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

I think that this is a breaking change. The pull request misses an update to the CHANGELOG.md, but I assume that this would warrant a major version bump.

The other approach would be to prioritize disable_frontmatter. If it's set to true, or if it's a function that returns true, then that should make disable_frontmatter_creation and disable_frontmatter_update default to true as well, even if they are configured.

Comment on lines 55 to +56
disable_frontmatter = false,
update_frontmatter = false,

Choose a reason for hiding this comment

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

  • The ClientOpts documentation update seems to signal that disable_frontmatter is no longer supported, yet it's set to false.
  • The ClientOpts documentation doesn't seem to mention update_frontmatter at all, instead it has disable_frontmatter_creation and disable_frontmatter_update.
  • The README.md file still references disable_frontmatter and it doesn't mentions the new settings.

@thetwistedlogic
Copy link
Author

I'm not a maintainer, but I skimmed through a couple of pull requests before planning to create one of my own and I left a couple of comments here that could be of use to improve this work.

Oh wow. Thank you so much! I've actually moved away from this plugin completely. I prefer something that is a simple cli tool with neovim integration instead. Currently developing a homebaked tool.

This plugin unfortunately destroys the frontmatter for my other notes since it does not support yaml tags; this distrupts my workflow personally.

Regardless, thank you.

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