-
Notifications
You must be signed in to change notification settings - Fork 145
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
accept triple slash file URIs (file:///...) as valid links #391
Conversation
sorry for the formatting in the PR text... Its a gh cli thing cli/cli#7616 |
This pull request has been automatically marked as stale because it has not had recent activity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
@mcDevnagh are you around to approve your reviewed changes (merging is blocked until you review)? Otherwise I'll dismiss and ask someone else to approve (just feels rude to dismiss until I try to ping you here as well). |
I'll take a look |
seems that @mcDavenagh isn't available enough at the moment to re-review, and @jurica already reviewed
@jurica , I think mcDevangh is a bit too snowed under to re-review. I've dismissed his review so that you can pass the review as well, incase he needs more time. Just caus I'd like to move on to tackling the other bugs this long weekend, which will be working in the same file as this PR. |
Sure if it's holding you back from doing more. I didn't want to merge it right away last week to give others the chance to also review... |
sorry for dropping the ball on this one! Thank you @jurica for picking up my slack. For what it's worth, code looks good :) |
PR to fix #250. Links such as
[title](file:///file.go)
were returning anlsp error "not found". The LSP does not actually check for the existence of
files via this format. Instead it was a symptom of the markdown link regex
checking, whereby the link was being understood as an internal markdown
document. Zk / lsp then tried to find the note
file:///file.example
, which itif course couldn't, and so it was throwing the "not found" error.
This sollution copies the logic already present for embedded images, which is to
pick a defining syntactic element of the link (in this case, the third
occurrence of a '/' rune), and to skip that link with
continue
, effectivelyignoring it.