Skip to content

Add support for references when no [] exists#144781

Merged
mjbvz merged 9 commits intomainfrom
unknown repository
Mar 17, 2022
Merged

Add support for references when no [] exists#144781
mjbvz merged 9 commits intomainfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Mar 9, 2022 •
edited by ghost
Loading

This PR fixes #141285

Code has been updated with the main branch and I have personally reviewed it.

Changes

Alters the regex to match [...] and [...][...]. Treats [...] as if it's the same as [...][].

Testing

On the feature branch:

  1. Create a new markdown file.
  2. Add in some references. For example: 
    [Works]
[Works][]
[Works][Works]
[Still Works][Works]

[Does Not Work]
[Does Not Work][]
[Does Not Work][DoesNotWork]
[Works][DoesNotWork] Jokes

[Works]

[Works]: https://microsoft.com
  3. Make sure that the links that you'd expect to work continue to work, both in the editor display and the preview.
    • Expected behavior in editor: Ctrl-Click (or Cmd-Click on Mac) brings cursor to reference.
    • Expected behavior in preview: References turn blue and link to their targets.
    • Preview can be opened via running Markdown: Open Preview from the Command Palette.

Expected Output

image

@ghost ghost changed the title add single reference linking regex Add support for references when no [] exists Mar 9, 2022
@mjbvz
Copy link
Collaborator

mjbvz commented Mar 9, 2022

Thanks for taking a look! Can you please also add tests for this:

vscode/extensions/markdown-language-features/src/test/documentLinkProvider.test.ts

Line 15 in 7ccb868

const testFile = vscode.Uri.joinPath(vscode.workspace.workspaceFolders![0].uri, 'x.md');

In the tests, also try adding test for potential edge cases or false positives where link detection should not occur

I'm also inclined to say that the link detection should not kick in for [Works]: https://microsoft.com. We already highlight the destination link in that case and highlighting the link id itself doesn't strike me as useful here

@ghost
Copy link
Author

ghost commented Mar 11, 2022 •
edited by ghost
Loading

Will do!

@ghost
Copy link

ghost commented Mar 16, 2022 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@ghost
Copy link
Author

ghost commented Mar 16, 2022

Sorry for the delay. Should be ready for review!

mjbvz
mjbvz requested changes Mar 16, 2022
View reviewed changes
Copy link
Collaborator

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

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

Looks reasonable overall. Thanks for following up!

Can you please remove the binary file added under ./scripts though? Looks like it was committed by mistake

@ghost
Copy link
Author

ghost commented Mar 16, 2022

Whoops! My bad for littering. Should be fixed, thanks for the heads up!

@ghost ghost requested a review from mjbvz March 16, 2022 21:33
@mjbvz mjbvz added this to the March 2022 milestone Mar 17, 2022
@mjbvz mjbvz merged commit b6f6a37 into microsoft:main Mar 17, 2022
@mjbvz
Copy link
Collaborator

mjbvz commented Mar 17, 2022

Thanks! Will be in the next insiders build

@github-actions github-actions bot locked and limited conversation to collaborators May 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mjbvz mjbvz Awaiting requested review from mjbvz

Assignees

@mjbvz mjbvz

Labels

None yet

Projects

None yet

Milestone

March 2022

Development

Successfully merging this pull request may close these issues.

Support references links when no [] exists

2 participants

@mjbvz @PandaGnar