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 support for references when no [] exists #144781

Merged
merged 9 commits into from Mar 17, 2022
Merged

Add support for references when no [] exists #144781

merged 9 commits into from Mar 17, 2022

Conversation

ghost
Copy link

@ghost ghost commented Mar 9, 2022

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:

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

Will do!

@ghost
Copy link

ghost commented Mar 16, 2022

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!

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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support references links when no [] exists
2 participants