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

Detect md image link #66958

Merged
merged 7 commits into from Jan 26, 2019

Conversation

Projects
None yet
2 participants
@flurmbo
Copy link
Contributor

flurmbo commented Jan 23, 2019

Fixes #49238. I added an option to the regex to have an image instead of text for the link description. Then when iterating over regex matches providerInlineLinks will now check to see if an image resource was also matched, and if so add it to results. All the tests pass, as well as a new one I've added.

Feedback on the regex would be appreciated, also I think perhaps the code I copied starting here could be refactored into a separate function.

@mjbvz mjbvz added this to the December/January 2019 milestone Jan 23, 2019

@mjbvz
Copy link
Contributor

mjbvz left a comment

Thanks for taking a look! Changes look good, just a few tweaks.

Let me know if you have any questions about my comments or the code

@@ -51,7 +51,7 @@ function matchAll(
}

export default class LinkProvider implements vscode.DocumentLinkProvider {
private readonly linkPattern = /(\[[^\]]*\]\(\s*)((([^\s\(\)]|\(\S*?\))+))\s*(".*?")?\)/g;
private readonly linkPattern = /(\[((!\[(.+)\]\()(.+)\)\]|[^\]]*\])\(\s*)((([^\s\(\)]|\(\S*?\))+))\s*(".*?")?\)/g;

This comment has been minimized.

@mjbvz

mjbvz Jan 23, 2019

Contributor

Regex looks good but I think it does need to be tweaked to be less greedy:

  • Instead of using . for matching the contents between the brackets, match on what that content cannot contain, such as [^\]] so that we match any character that is not a square bracket.
  • Use a non-greedy quanitfier: +? instead of just + so we consume the minimum number of characters that still meet the regexp

The reason is that for markdown files like:

[![alt text](image.jpg)](https://example.com) other text [![alt text](image.jpg)](https://example.com)

if the matching is greedy, the detected link could end up spanning both links.

This comment has been minimized.

@mjbvz

mjbvz Jan 23, 2019

Contributor

Probably worth adding a test for that example md as well

Show resolved Hide resolved extensions/markdown-language-features/src/features/documentLinkProvider.ts Outdated

@mjbvz mjbvz merged commit 4fe1cdc into Microsoft:master Jan 26, 2019

2 checks passed

VS Code #20190126.1 succeeded
Details
license/cla All CLA requirements met.
Details
@mjbvz

This comment has been minimized.

Copy link
Contributor

mjbvz commented Jan 26, 2019

Thanks! This change will be in the next VS Code insiders build and is scheduled to be included in VS Code 1.31

@flurmbo

This comment has been minimized.

Copy link
Contributor Author

flurmbo commented Jan 26, 2019

Awesome, thanks for the fast response

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment