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

Add Wikilink titles support #221

Merged
merged 3 commits into from
Jun 20, 2023
Merged

Conversation

eguiraud
Copy link
Contributor

@eguiraud eguiraud commented Jun 15, 2023

With this patch wikilinks of the form [[doc#heading|title]] are correctly recognized as links to doc#heading.
Also go-to-definition works as normal.
Fixes #136.

Admittedly, this could have been done with a smaller patch, but the addition of the title-handling logic was making the function quite hard to follow (already before the patch it had 6+ levels of indentation).
So I took this chance to refactor the parsing to use a simple "functional finite state machine pattern". Basically we have a parse() function that kicks off the parsing and then transitions to other functions as needed via pattern matching (parseTitle, parseHeading, parseDoc, parseEnd).

  • parseDoc can transition to all other functions but not to parse (we don't go back to the beginning) or fail early
  • parseHeading can only transition to parseTitle or parseEnd (we don't go back to the doc) or fail early
  • parseTitle can only transition to parseEnd or fail early
  • parseEnd can either return success (this was a well-formed wiki link) or failure (not a well-formed wiki link)

I hope the new logic is easy to follow!

@artempyanykh
Copy link
Owner

So I took this chance to refactor the parsing to use a simple "functional finite state machine pattern"

Love it, thanks!

I'll have a closer look shortly. Meanwhile, could you please

  1. run make fmt and make check to unbork the CI, and
  2. add a test or two in ParserTests.fs to exercise the new code path.

With this patch wikilinks of the form [[doc#heading|title]]
are correctly recognized as links to doc#heading.
Also go-to-definition works as normal.

This fixes artempyanykh#136.
@eguiraud
Copy link
Contributor Author

I applied the suggestions from make fmt and make check.

I need to spend some time understanding how the tests are set up, I'll try to do so soon.

Unrelated: while testing these changes interactively I found a problem that was not caught by the tests when suggesting completions for an empty [[]] wiki link, maybe there is a missing test case.

Copy link
Owner

@artempyanykh artempyanykh left a comment

Choose a reason for hiding this comment

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

A couple nits, otherwise looks great! @eguiraud LMK if you need some help with testing.

Marksman/Parser.fs Show resolved Hide resolved
Marksman/Parser.fs Outdated Show resolved Hide resolved
Marksman/Parser.fs Outdated Show resolved Hide resolved
Marksman/Parser.fs Outdated Show resolved Hide resolved
eguiraud added 2 commits June 19, 2023 16:39
- better names for things
- make contentAndSpan more readable
@eguiraud
Copy link
Contributor Author

Applied suggestions from code review and added some tests, although I still don't understand how the tests works 😅 first time I write F#, sorry

@artempyanykh
Copy link
Owner

Great job, thanks @eguiraud!

Re: tests -- I use snapshot tests quite often (also known as golden testing) where the test output is compared to a previously recorded output. I use https://github.com/theramis/Snapper library, which works albeit a bit fiddly wrt. updating the snapshots.

@artempyanykh artempyanykh merged commit 35bdaeb into artempyanykh:main Jun 20, 2023
@tjex
Copy link

tjex commented Jun 22, 2023

@eguiraud It doesn't seem to be working for me :(
updated via Mason in neovim to latest and tried to make two fresh links following the wikilink syntax.
you can see by the lack of lsp error that marksman does see the note at [[open source]].

the cwd of neovim is the same folder where the [[open source]] note is as well as where the test note is (being used to test the links below)

Screen Shot 2023-06-22 at 14 24 22

@artempyanykh
Copy link
Owner

@tjex it hasn't been released yet, just merged into main.

I'll cut a new release in a couple days. Otherwise, you may want to try building from source.

@eguiraud eguiraud deleted the wikilink-titles branch June 22, 2023 16:03
@tjex
Copy link

tjex commented Jun 22, 2023

ahh! apologies!

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.

Support wiki links titles
3 participants