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

fix(obsidian-nvim): added obsidian.nvim config and changed the readme #378

Merged
merged 9 commits into from
Jul 9, 2023

Conversation

mortang2410
Copy link
Contributor

@mortang2410 mortang2410 commented Jul 7, 2023

As requested from #309. We need this config to follow links to other markdown files. Please check if anything is amiss.

Edit: I see the review checklist asks for "Proper usage of opts table rather than setting things up with the config function." I'm not sure if that is satisfied here.

@github-actions
Copy link

github-actions bot commented Jul 7, 2023

Review Checklist

Does this PR follow the [Contribution Guidelines](development guidelines)? Following is a partial checklist:

Proper conventional commit scoping:

  • If you are adding a new plugin, the scope would be the name of the category it is being added into. ex. feat(utility): added noice.nvim plugin

  • If you are modifying a pre-existing plugin or pack, the scope would be the name of the plugin folder. ex. fix(noice-nvim): fix LSP handler error

  • Pull request title has the appropriate conventional commit type and scope where the scope is the name of the pre-existing directory in the project as described above

  • README is properly formatted and uses fenced in links with <url> unless they are inside a [title](url)

  • Proper usage of opts table rather than setting things up with the config function.

Uzaaft
Uzaaft previously requested changes Jul 7, 2023
lua/astrocommunity/note-taking/obsidian-nvim/init.lua Outdated Show resolved Hide resolved
@owittek
Copy link
Member

owittek commented Jul 7, 2023

Wouldn't it be enough to just add your 2 changes to the init.lua instead of pasting the entire default config as you're not changing the defaults?

@mortang2410
Copy link
Contributor Author

just add your 2 changes

Setting up the gf keybinding, for instance, to follow links is done in the config, so we have to copy paste some of it. I also disabled a few things like the notes, daily_notes, tabular, vim-markdown (as we already use treesitter), new note ID (it would create new notes upon the slightest mistypes etc.). I think those are sane defaults but please feel free to discuss.

@owittek
Copy link
Member

owittek commented Jul 7, 2023

Pardon me then, I came from the issue and the first part as well as the comments seem to be directly taken from the obsidian README.

But my point stands, it's unclear for me what parts deviate from the default config if you also put in all the defaults and leave all the comments in.

Also we'd have to handle line 91-95 in a way that is OS agnostic. For that we'd have to use a function for opts instead.

Lastly we should setup the key bind in a different way than to place it in the config function for various reasons.

@mortang2410
Copy link
Contributor Author

Also we'd have to handle line 91-95 in a way that is OS agnostic. For that we'd have to use a function for opts instead.
Lastly we should setup the key bind in a different way than to place it in the config function for various reasons.

I agree with both of these points. I admit, however, that I am unfamiliar with how astronvim should handle both of these. If you have solutions for them, please share.

@owittek
Copy link
Member

owittek commented Jul 7, 2023

I can refactor opts when I got time and with the key bind part perhaps @RayJameson could help since he did that kind of stuff for harpoon before.

@RayJameson
Copy link
Contributor

RayJameson commented Jul 8, 2023

Shouldn't it work with the keys property in the plugin setup? I did that thing for Harpoon because there is a top-level empty mapping with a description, that shows in <leader> which-key where are also other nested keymaps, so I wanted the Harpoon keymaps group to align with others, otherwise, I would stick to using keys

@owittek owittek requested a review from a team July 8, 2023 07:52
@owittek
Copy link
Member

owittek commented Jul 8, 2023

Hmm there's still an issue with the setup handler in my code. Testing right now...

@owittek owittek requested a review from Uzaaft July 8, 2023 08:57
Copy link
Member

@owittek owittek left a comment

Choose a reason for hiding this comment

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

I like my own changes, soo ...

@owittek owittek requested review from Uzaaft and removed request for Uzaaft July 9, 2023 11:53
@Uzaaft
Copy link
Member

Uzaaft commented Jul 9, 2023

@owittek Added a suggestion for windows support as well.

Copy link
Member

@owittek owittek left a comment

Choose a reason for hiding this comment

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

the pipeline does not like us

lua/astrocommunity/note-taking/obsidian-nvim/init.lua Outdated Show resolved Hide resolved
@Uzaaft
Copy link
Member

Uzaaft commented Jul 9, 2023

@owittek Apparently utils.system_open should be enough

Copy link
Member

@mehalter mehalter left a comment

Choose a reason for hiding this comment

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

lua/astrocommunity/note-taking/obsidian-nvim/init.lua Outdated Show resolved Hide resolved
@mehalter mehalter dismissed their stale review July 9, 2023 13:06

Resolved

Copy link
Member

@mehalter mehalter left a comment

Choose a reason for hiding this comment

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

LGTM

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.

5 participants