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(lsp): add 'zk.link' LSP command #284

Merged
merged 5 commits into from
Jan 18, 2023
Merged

Conversation

psanker
Copy link
Contributor

@psanker psanker commented Jan 10, 2023

Allows users to take advantage of the linking mechanism in their editing tools outside of the 'zk.new' context. For example, I made a "ZkInsertLink" command with Neovim that inserts a link at the current cursor location after searching for the desired note. This improves on the current behavior which only autocompletes based on note name, which users may or may not remember.

I looked for a CONTRIBUTING.md but couldn't find. I hope you find this PR useful!

Changes

  • Adds 'zk.link' LSP command
  • Refactors linking mechanism from executeCommandNew into utility function to be used by both executeCommandNew and executeCommandLink

psanker and others added 2 commits January 9, 2023 20:11
Allows users to take advantage of the linking mechanism in their editing
tools outside of the 'zk.new' context. For example, I made a
"ZkInsertLink" command that inserts a link at the current cursor location
after searching for the desired note. This improves on the current
behavior which only autocompletes based on note name, which users may or
may not remember.
Copy link
Member

@mickael-menu mickael-menu left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, this will be very useful!

Would you mind updating the documentation to add the new zk.link command?

internal/adapter/lsp/cmd_link.go Outdated Show resolved Hide resolved
internal/adapter/lsp/cmd_link.go Outdated Show resolved Hide resolved
internal/adapter/lsp/util.go Outdated Show resolved Hide resolved
@mickael-menu
Copy link
Member

Could you also git apply the following patch to update the changelog? I don't have the rights to push on your fork directly. (If you open another PR, you can check the "allow edits from maintainers" checkbox when creating the PR). Thanks!

changelog.patch

Some extra bool values were previously returned in linkNote and the
zk.link LSP command. These values were vestigial placeholders from
the initial refactor, in case extra information was useful. It turns out
this was not the case.
Also updates CHANGELOG to reflect the zk.link contribution
@psanker
Copy link
Contributor Author

psanker commented Jan 18, 2023

Done! Added your suggestions and the changelog patch. As I note in the commits, the bool values were vestigial values, originally there if there was extra information that could have been useful to send back. Turns out there wasn't really.

I also have this LSP command used in my patch to zk-nvim: zk-org/zk-nvim@main...psanker:zk-nvim:better-linking

And I have some example commands in my dotfiles that use zk.link: https://github.com/psanker/dotfiles/blob/37e6f700157b0a3325c7213cf132af3ab57d90b3/nvim/.config/nvim/lua/psanker/zk.lua#L4-L79

Would you be interested in a PR for these on zk-nvim?

@mickael-menu
Copy link
Member

Looks like the build fails with the recent changes.

GOARCH=arm64 go build -tags "fts5" -ldflags "-X=main.Version=`git describe --tags --match v[0-9]* 2> /dev/null` -X=main.Build=`git rev-parse --short HEAD`"
# github.com/mickael-menu/zk/internal/adapter/lsp
internal/adapter/lsp/cmd_new.go:93:20: assignment mismatch: 2 variables but linkNote returns 1 value
make: *** [build] Error 2

Would you be interested in a PR for these on zk-nvim?

Sure! That would be useful. Thanks again.

@psanker
Copy link
Contributor Author

psanker commented Jan 18, 2023

Whoops! Thought I caught all of those refactors. make install now passes cleanly for me.

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