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(lsp): ignore diagnostic check within code blocks #399

Merged
merged 10 commits into from
Apr 6, 2024

Conversation

Rahlir
Copy link
Contributor

@Rahlir Rahlir commented Apr 2, 2024

resolves: #194.
resolves: zk-org/zk-nvim#161

I tried to be as thorough as possible, let me know if I missed any other markdown ways to denote code blocks. I am also including here two files I have used to test this fix:
empty-note.md
test-note.md

@tjex tjex self-requested a review April 3, 2024 07:36
@tjex
Copy link
Member

tjex commented Apr 3, 2024

awesome! Thank you! I'll have a look at this tomorrow (or latest day after)

@tjex
Copy link
Member

tjex commented Apr 4, 2024

@Rahlir
Can confirm that links within code blocks are ignored, as are links within inline back ticks and indented code; with one exception:

16 |  `[[this is a link existing as the first string on a line]]`     ■ not found

These are getting missed for some reason. Will keep inspecting, but just wanted to chime in as I go.

Thanks again for this!

I do however think we can be clearer with the function and variable naming though.
linkWithinInlineCode does in fact apply to code within inline back ticks as well as code blocks.

I would suggest simply linkWithinBackticks / linkWithinCode instead. Thoughts?

@Rahlir
Copy link
Contributor Author

Rahlir commented Apr 4, 2024

Thanks for the feedback!

Hmm, weird that the first string is being missed, that's probably some oversight with 0 index, will fix that asap. Thanks for catching that.

Regarding the function names: I am not entirely sure what you mean. The function linkWithinInlineCode only checks whether the link is within backticks. The code block checks are separate checks done with regexes in the beginning of the for loop over lines. Or did I misunderstand what you meant?

@tjex
Copy link
Member

tjex commented Apr 4, 2024

Found it :) It's because when the link is the first string in the line, the backtick == 0.

So backtickId > 0 && backtickId < linkEnd needs to be backtickId >= 0 && backtickId < linkEnd

Regarding the naming, that was my confusion! I thought the function was being used for code blocks as well.
The comment perhaps (partially) threw me off (by my expertise had me already on a pretty diagonal course!)

Perhaps // Recursive function to check whether a link is within inline code block. could be changed to // Recursive function to check whether a link is within inline code.?

internal/adapter/lsp/document.go Outdated Show resolved Hide resolved
internal/adapter/lsp/document.go Outdated Show resolved Hide resolved
Copy link
Member

@tjex tjex left a comment

Choose a reason for hiding this comment

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

great! Thanks for getting these changes back so quickly.

I'm going to give it a second pass with a fresh mind this morning, just to double check myself.

@tjex
Copy link
Member

tjex commented Apr 5, 2024

@Rahlir I pushed the 'fixes' from my comments, so you can just pull to check them out if you like. If my comments/changes are misguided, let me know and I'll revert!

@Rahlir
Copy link
Contributor Author

Rahlir commented Apr 5, 2024

I think moving the check is fine and you are right that it might shave some execution time off.

Hardcoding insideInline = true is wrong though. The inversion is there because if you have three ticks on one line, the next line is still an inline comment, then if you have three backticks on next line, the third backtick ends the inline comment, it doesn't start it. That's why there is an inversion.

@tjex
Copy link
Member

tjex commented Apr 5, 2024

ahhh yep, awesome, thanks for taking the time to explain to me 👌

@Rahlir
Copy link
Contributor Author

Rahlir commented Apr 5, 2024

No problem 🙂. All looks good to me now.

@tjex tjex merged commit c9d4734 into zk-org:main Apr 6, 2024
3 checks passed
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.

Incorrect "not found" diagnostic LSP server is matching links inside fenced code blocks
2 participants